Remove infobar notification from pdf viewer
Categories
(Firefox :: PDF Viewer, task)
Tracking
()
People
(Reporter: calixte, Assigned: calixte)
References
Details
(Whiteboard: [proton-uplift] [proton-infobars] [priority:2c])
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
See https://bugzilla.mozilla.org/show_bug.cgi?id=1705060#c2:
UX recommends not showing this infobar notification unless there is a significant security reason to include it
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Recommandation from UX is to not show this bar, so remove it.
Pushed by cdenizet@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a802bfb09181 Remove infobar notification from pdf viewer r=bdahl
Comment 3•4 years ago
|
||
Backed out for failures on browser_pdfjs_notification_close_on_navigation.js
backout: https://hg.mozilla.org/integration/autoland/rev/b0887ad1341fa9e85b7bfd4bcf007cc912937aac
failure log: https://treeherder.mozilla.org/logviewer?job_id=336718237&repo=autoland&lineNumber=2707
[task 2021-04-15T22:53:52.327Z] 22:53:52 INFO - TEST-INFO | screentopng: exit 0
[task 2021-04-15T22:53:52.328Z] 22:53:52 INFO - Buffered messages logged at 22:53:44
[task 2021-04-15T22:53:52.329Z] 22:53:52 INFO - Entering test bound test_notification_is_removed_upon_navigation
[task 2021-04-15T22:53:52.332Z] 22:53:52 INFO - Buffered messages finished
[task 2021-04-15T22:53:52.333Z] 22:53:52 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/pdfjs/test/browser_pdfjs_notification_close_on_navigation.js | Uncaught exception - waiting for notification - timed out after 50 tries.
[task 2021-04-15T22:53:52.334Z] 22:53:52 INFO - Leaving test bound test_notification_is_removed_upon_navigation
[task 2021-04-15T22:53:52.336Z] 22:53:52 INFO - GECKO(1534) | MEMORY STAT | vsize 3127MB | residentFast 334MB | heapAllocated 91MB
[task 2021-04-15T22:53:52.337Z] 22:53:52 INFO - TEST-OK | toolkit/components/pdfjs/test/browser_pdfjs_notification_close_on_navigation.js | took 7733ms
Comment 4•4 years ago
|
||
Also failing: TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - TEST-PASS | dom/base/test/test_user_select.html | testB: range[0].endOffset
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - Buffered messages finished
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - checkText@dom/base/test/test_user_select.html:108:7
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - test@dom/base/test/test_user_select.html:242:12
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: Selection range count - got +0, expected 3
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - checkRangeCount@dom/base/test/test_user_select.html:127:7
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - test@dom/base/test/test_user_select.html:243:18
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | uncaught exception - IndexSizeError: Selection.getRangeAt: 0 is out of range at checkRange@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:132:17
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - test@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:244:13
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO -
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - simpletestOnerror@SimpleTest/SimpleTest.js:2112:18
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - GECKO(1553) | JavaScript error: http://mochi.test:8888/tests/dom/base/test/test_user_select.html, line 132: IndexSizeError: Selection.getRangeAt: 0 is out of range
[task 2021-04-15T23:31:24.792Z] 23:31:24 INFO - GECKO(1553) | MEMORY STAT | vsize 20974971MB | residentFast 1350MB
[task 2021-04-15T23:31:24.792Z] 23:31:24 INFO - TEST-OK | dom/base/test/test_user_select.html | took 507ms
Pushed by cdenizet@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0851da1d6c1d Remove infobar notification from pdf viewer r=bdahl
Assignee | ||
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Backed out for causing mochitest plain failures
Backout link: https://hg.mozilla.org/integration/autoland/rev/120aba36a06e7ae6dbd608e7791b139f25f01878
INFO - Buffered messages finished
[task 2021-04-16T08:46:31.750Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"
[task 2021-04-16T08:46:31.750Z] 08:46:31 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-16T08:46:31.751Z] 08:46:31 INFO - checkText@dom/base/test/test_user_select.html:108:7
[task 2021-04-16T08:46:31.751Z] 08:46:31 INFO - test@dom/base/test/test_user_select.html:242:12
[task 2021-04-16T08:46:31.752Z] 08:46:31 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-16T08:46:31.753Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: Selection range count - got +0, expected 3
[task 2021-04-16T08:46:31.754Z] 08:46:31 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-16T08:46:31.754Z] 08:46:31 INFO - checkRangeCount@dom/base/test/test_user_select.html:127:7
[task 2021-04-16T08:46:31.755Z] 08:46:31 INFO - test@dom/base/test/test_user_select.html:243:18
[task 2021-04-16T08:46:31.755Z] 08:46:31 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-16T08:46:31.756Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | uncaught exception - IndexSizeError: Selection.getRangeAt: 0 is out of range at checkRange@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:132:17
[task 2021-04-16T08:46:31.756Z] 08:46:31 INFO - test@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:244:13
Assignee | ||
Comment 7•4 years ago
|
||
:sandor, I don't think this failure is related to the patch I landed.
When I open log and then I open the job for this log I get:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=089fd9e33fe26d5b4215692f84493b2026a071f2&selectedTaskRun=GMMNMWOlQAOVLTGhebnXYg.0
I can see this failing job:
https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=AGz6wbbxRfm3ynvhBgtYug.0
but its name 16-089fd9e33fe2-bk
seems to indicate again an issue with the above patch.
Could you fix that please ?
Comment 8•4 years ago
•
|
||
That log is from where we discovered the failures.
You can see in this range,that the test under your patch are green and they are starting to fail after your patch
The name is from the backfilling action task. it shows from what change set the backfill started.
You can read about this feature here
Assignee | ||
Comment 9•4 years ago
|
||
:sandor, ok I understand the backout.
I'm pretty sure that this patch is not the real culprit since it just aims to remove an infobar from pdf reviewer and there nothing related to text selection here.
Assignee | ||
Comment 10•4 years ago
|
||
So there is something wrong with tests here:
https://searchfox.org/mozilla-central/source/dom/base/test/mochitest.ini
I just disabled the two tests related to pdf and I got the same error:
https://treeherder.mozilla.org/jobs?repo=try&revision=49d1a46d59b564305924db02204f038f53f673d7
So my patch is not the root cause of the regression but just highlights something wrong.
This test: https://searchfox.org/mozilla-central/source/dom/base/test/test_pdf_print.html
loads a bad pdf and so two (??) infobars are displayed.
So my patch implies that these infobars aren't displayed anymore (the innerHeight property isn't the same).
Since I can reproduce locally, I made a try in keeping only pdf tests and test_user_select
in the mochitest.ini
and of course everything runs fine !? so something else is wrong.
Using luck and dichotomy, I managed to get similars errors with test_user_select
with this basic mochitest.ini
:
[test_meta_viewport_fixed_width_and_zero_display_width.html]
[test_user_select.html]
:botond, would you have an idea here ?
Comment 11•4 years ago
|
||
I can reproduce the failure with those two tests run in sequence, and it certainly looks weird visually (as if the whole harness page briefly zooms in, although I'm not seeing an actual change to the resolution that would result in that).
I will try to get to the bottom of it, but if you need a quick fix in the meantime, moving the test_meta_viewport_*
tests into their own subdirectory should help.
Comment 12•4 years ago
|
||
Investigation so far shows that, at the beginning of test_user_select.html
, the top-level content window (the one containing the mochitest harness) has a nonzero scroll position. That scroll seems to come from here. Not sure yet why the previous test affects whether or not that happens.
Comment 13•4 years ago
|
||
Seems to be related to test_meta_viewport_fixed_with_and_zero_display_width.html
changing layout.css.devPixelsPerPx via scaleRatio()
here. This does cause the whole test harness to zoom in, albeit using full-zoom rather than resolution.
Comment 14•4 years ago
|
||
Ok, so the logical fix seems to be to pair this call to pushPrefEnv()
(which changes layout.css.devPixelsPerPx
) with a corresponding call to popPrefEnv()
at the end of the test.
However, this does not seem to have the desired effect. While the device pixel ratio is changed back to 60, and we get e.g. a call to AppUnitsPerDevPixelChanged()
for it, the visible area of the root content document's pres shell remains stuck at a size corresponding to the old ratio, and while the root content doc does get reflowed, it's reflowed into this stale size.
I'm not sure what is supposed to trigger the updating of the pres shell's visible area and a reflow to the correct size after the device pixel ratio changes. cc'ing Emilio in case he has some ideas.
Perhaps changing layout.css.devPixelsPerPx
, which is an action that affects the mochitest harness document itself, is not a well-behaved thing for a mochitest to be doing in the first place?
Comment 15•4 years ago
|
||
Pushed by cdenizet@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/45376b7c2ff5 Remove infobar notification from pdf viewer r=bdahl
Comment 16•4 years ago
|
||
bugherder |
Assignee | ||
Comment 17•4 years ago
|
||
:RT, do you want to have this patch in 89 ?
Comment 18•3 years ago
|
||
(In reply to Calixte Denizet (:calixte) from comment #17)
:RT, do you want to have this patch in 89 ?
Yes please, the infobar shows currently on 89, which is counter to our proton goals so can we please consider this as part of the proton uplifts?
Assignee | ||
Comment 19•3 years ago
|
||
Comment on attachment 9216037 [details]
Bug 1705327 - Remove infobar notification from pdf viewer
Beta/Release Uplift Approval Request
- User impact if declined: User will continue to see the infobar when opening some specific pdfs.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: Bug 1706021
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Because it just removes an infobar.
- String changes made/needed:
Comment 20•3 years ago
|
||
Comment on attachment 9216037 [details]
Bug 1705327 - Remove infobar notification from pdf viewer
Looks low risk, has tests and baked a few days on nightly, approved for 89 beta 5, thanks.
Updated•3 years ago
|
Comment 21•3 years ago
|
||
bugherder uplift |
Comment 22•3 years ago
|
||
Looks like this was a dupe of bug 1701006, copying its proton flags over
Comment 24•3 years ago
|
||
Verified as fixed using the latest Nightly 90.0a1 and the latest Firefox 89 beta 9 - the PDF Infobar notification has been removed. Verified on macOS BigSur 11, Ubuntu 20.04, and Windows 10.
Description
•