Fix rendering problem when zooming on Safari

[Aditi-1400]

Fixes: #16155
@calixteman suggestion in the comment #17571 (comment), works well for iPad/iOS devices.
Before the fix:

Before.mp4

After the fix:

After.mp4

[timvandermeij]

/botio-linux preview

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/8e9ebf326bd7518/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/8e9ebf326bd7518/output.txt

Total script time: 1.18 mins

Published

• Viewer: http://54.241.84.105:8877/8e9ebf326bd7518/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/8e9ebf326bd7518/legacy/web/viewer.html

[timvandermeij]

/botio test

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/0707513770b2969/output.txt

[moz-tools-bot]

From: Bot.io (Windows)

---

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/7bc3aed9d04a96d/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Failed

Full output at http://54.241.84.105:8877/0707513770b2969/output.txt

Total script time: 27.09 mins

• Unit tests: Passed
• Integration Tests: Passed
• Regression tests: FAILED

  different ref/snapshot: 25
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/0707513770b2969/reftest-analyzer.html#web=eq.log

[moz-tools-bot]

From: Bot.io (Windows)

---

Failed

Full output at http://54.193.163.58:8877/7bc3aed9d04a96d/output.txt

Total script time: 42.95 mins

• Unit tests: Passed
• Integration Tests: FAILED
• Regression tests: FAILED

  different ref/snapshot: 3

Image differences available at: http://54.193.163.58:8877/7bc3aed9d04a96d/reftest-analyzer.html#web=eq.log

[calixteman]

From my point of view, this change is fine but I'd prefer having @Snuffleupagus agreement here.

[timvandermeij]

The change looks good to me, but I prefer we merge this after the April release (which I'll do either today or tomorrow) given the nature of the change so it has quite some time to settle before the next release.

[Snuffleupagus]

Personally I'm really not excited about a browser-specific hack (also given the age of this code), especially considering that Safari has never been listed as fully supported; note https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

(Given the comments just above I suppose that I'm overruled here though...)

[timvandermeij]

If it would have involved adding code I also wouldn't have been in favor of it, but if the issue can be solved with removing a line (which also shortens the CSS ever so slightly) then overall this seems acceptable to me. However, I should add that that is under the assumption that the tests have good coverage and no regressions are discovered; if those are discovered we won't attempt to find a fix that pleases Safari and instead just revert the change, so it's on a best-effort basis and no guarantees for Safari are given (which applies in general for Safari given the FAQ support entry and the fact that Safari consistently lags behind other browsers).

[calixteman]

I agree that in fine, it's a fix for Safari, but I don't think that removing a "useless" property could cause a problem here and it could be considered as a kind of clean up/simplification which is kinda useful for Firefox either.
I quoted useless because I don't know how to make it useful (which doesn't mean that it's really useless).
@Snuffleupagus, you aren't overruled at all and if you can imagine a way to make this property useful in a specific context, please share.
That said if it causes a regression, I won't hesitate one nanosecond to backout it in considering that Firefox is always our priority.

[Snuffleupagus]

However, I should add that that is under the assumption that the tests have good coverage and no regressions are discovered;

I don't know if this is actually covered by tests, given that the code only applies to the viewer. (Unless a change here would just happen to trigger an "unrelated" integration-test.)
Probably the only thing that works as far as testing goes is careful manual checking of the viewer (using e.g. all the various scroll/spread-modes and using both LTR- and RTL-locales); something that I unfortunately don't have time for personally.

if you can imagine a way to make this property useful in a specific context, please share.

I have nothing in mind, I just want us to avoid wasting time chasing a regression later on :-)

---

If you both are satisfied that this is correct, and you've tested this thoroughly, then I retract my previous objection.

[timvandermeij]

I have performed extra manual checking of the viewer today, with e.g. the scroll/spread modes, LTR/RTL locales, rotation and the cursor tools, and I haven't found any change in behavior compared to the viewer on the master branch. Given that there are also no unexpected integration tests failures, I share @calixteman's analysis that this change should be safe. Moreover, now that we have a fresh release from yesterday this should have enough time to settle, and should the situation arise where a regression is discovered then we'll back this out.

[Snuffleupagus]

That said if it causes a regression, I won't hesitate one nanosecond to backout it in considering that Firefox is always our priority.

Apparently this did cause a regression, see bug 1895909, hence we should just revert this!

[nicolo-ribaudo]

We wrongly assumed that .pdfViewer .page canvas was only targeting the PDF pages canvases and nothing else, apologies 😅