1591947 - Bookmarks Menus show incorrect scroll arrow icons

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[M8R-p7]

Attachment: BMB-scrollarrows.png

I've selected 'Linux' as the platform, but have only tested on Fedora 30 and Windows 10. I cannot reproduce this on Windows.
I've selected 71 Branch as I cannot reproduce in Release 70. It is present in Developer Edition (71.0b4) and Nightly (72.0a1).

STR:
0. Create a new profile

1. Import or create enough bookmark items to ensure scrolling (and some folders with the same)
2. Enter 'Customize' mode and add the 'Bookmarks Menu' button to the toolbar.
3. Click the button

Expected results:
The bookmarks menu panel is shown, with arrow buttons at the top and bottom to assist scrolling. Scrollable submenus would similarly show arrow buttons when scrolling is necessary.

Actual Results:
The arrow button icons are there, but have a Bookmarks-Star-On-Tray icon on top. Folder submenus show a arrow with Folder icon.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Wait is this temporary or permanent?

Comment 3

[M8R-p7]

Permanent - it's not a Flash-Of-Unstyled-Content or similar. Opening the Browser Toolbox isn't required.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Then this is not bug 1590280.

Comment 5

[M8R-p7]

mozregression says:

No more inbound revisions, bisection finished.
Last good revision: 613e9a1a99f34df0fde97c5ed6c6b5ed268915d8
First bad revision: 7c3489dfee6082bec00f05d0f02f502ec4686743
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=613e9a1a99f34df0fde97c5ed6c6b5ed268915d8&tochange=7c3489dfee6082bec00f05d0f02f502ec4686743

There's only one changeset in that range, which is for bug1397876 .

Comment 6

[Dão Gottwald [:dao]]

(In reply to M8R-p7 from comment #5)

mozregression says:

No more inbound revisions, bisection finished.
Last good revision: 613e9a1a99f34df0fde97c5ed6c6b5ed268915d8
First bad revision: 7c3489dfee6082bec00f05d0f02f502ec4686743
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=613e9a1a99f34df0fde97c5ed6c6b5ed268915d8&tochange=7c3489dfee6082bec00f05d0f02f502ec4686743

There's only one changeset in that range, which is for bug1397876 .

Thanks!

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Then this is not bug 1590280.

Indeed it should be caused by something else than bug 1590280 (since this one is a permanently broken visual appearance), but the bug description sounds quite similar to the observations from 1590280 comment 2, i.e it looks like list-style-image style is propagated from parent menuitem to shadow DOM toolbarbutton of arrowscrollbox element. I don't see anything linux specific in styles though (https://phabricator.services.mozilla.com/D35040), so sort of surprised to see the bug is reported for linux only. Also I have no a ready-to-check linux machine at hands, Alice, just in case, is there any luck to come with reliable strs for win or osx?

Comment 8

[Alice0775 White]

(In reply to alexander :surkov (:asurkov) from comment #7)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Then this is not bug 1590280.

Indeed it should be caused by something else than bug 1590280 (since this one is a permanently broken visual appearance), but the bug description sounds quite similar to the observations from 1590280 comment 2, i.e it looks like list-style-image style is propagated from parent menuitem to shadow DOM toolbarbutton of arrowscrollbox element. I don't see anything linux specific in styles though (https://phabricator.services.mozilla.com/D35040), so sort of surprised to see the bug is reported for linux only. Also I have no a ready-to-check linux machine at hands, Alice, just in case, is there any luck to come with reliable strs for win or osx?

Nothing special STR. I can easily reproduce the issue on Linux Mint 19 X-Cinnamon. And I got same regression window of comment#5.

However, no luck on Windows10.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Oh, shoot, I can repro trivially as well.

The image is inheriting list-style-image from this rule: https://searchfox.org/mozilla-central/rev/d061ba55ac76f41129618d638f4ef674303ec103/browser/themes/shared/toolbarbutton-icons.inc.css#192

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This is a silly layout bug, combined with the lack of bug 1584935 which makes us construct the frames too early while the style is outdated.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1591947 - Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin

Consider the following case:

<image style="list-style-image: url(foo.png)"></image>

image.style.MozAppearance = "something"

The early return was preventing us from clearing the image.

This is an ancient bug, but it has started happening in the browser chrome
because the lack of lazy frame construction for XUL elements makes us construct
elements with an outdated style, which means in this case that they wouldn't
have the -moz-appearance rule applied yet.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1aae03a822b8
Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin

Comment 13

[Alexandru Michis [:malexandru]]

Backed out changeset 1aae03a822b8 for causing reftest failures in image-appearance-dynamic.xul

Backout link: https://hg.mozilla.org/integration/autoland/rev/6d393135fdb9709fea22203fd3bb4b772db1d868

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=1aae03a822b891ef346603d537c271e5c9b8c677&tochange=6d393135fdb9709fea22203fd3bb4b772db1d868&searchStr=reftest%2Cwindows&failure_classification_id=2&selectedJob=275313667

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=275313667&repo=autoland&lineNumber=56374

[task 2019-11-08T13:37:22.428Z] 13:37:22 INFO - REFTEST TEST-START | chrome://reftest/content/xul/image-appearance-dynamic.xul == chrome://reftest/content/xul/image-appearance-dynamic-ref.xul
[task 2019-11-08T13:37:22.428Z] 13:37:22 INFO - REFTEST TEST-LOAD | chrome://reftest/content/xul/image-appearance-dynamic.xul | 57 / 76 (75%)
[task 2019-11-08T13:37:22.443Z] 13:37:22 INFO - ++DOMWINDOW == 116 (10E6C800) [pid = 5756] [serial = 175] [outer = 011C7940]
[task 2019-11-08T13:37:22.550Z] 13:37:22 INFO - [Child 5756, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file z:/build/build/src/dom/base/nsContentUtils.cpp, line 3719
[task 2019-11-08T13:37:22.632Z] 13:37:22 INFO - REFTEST TEST-LOAD | chrome://reftest/content/xul/image-appearance-dynamic-ref.xul | 57 / 76 (75%)
[task 2019-11-08T13:37:22.647Z] 13:37:22 INFO - ++DOMWINDOW == 117 (0D46B800) [pid = 5756] [serial = 176] [outer = 011C7940]
[task 2019-11-08T13:37:22.747Z] 13:37:22 INFO - [Child 5756, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file z:/build/build/src/dom/base/nsContentUtils.cpp, line 3719
[task 2019-11-08T13:37:22.877Z] 13:37:22 INFO - REFTEST TEST-UNEXPECTED-FAIL | chrome://reftest/content/xul/image-appearance-dynamic.xul == chrome://reftest/content/xul/image-appearance-dynamic-ref.xul | image comparison, max difference: 255, number of differing pixels: 128
[task 2019-11-08T13:37:22.877Z] 13:37:22 INFO - REFTEST IMAGE 1 (TEST):
[task 2019-11-08T13:37:22.877Z] 13:37:22 INFO - REFTEST IMAGE 2 (REFERENCE):

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74ae5427c21f
Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/74ae5427c21f

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9107073 [details]
Bug 1591947 - Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0 (Mac and Linux only)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix longstanding dynamic change handling bug that was uncovered by the shadow dom migration.
• String changes made/needed: none

Comment 17

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9107073 [details]
Bug 1591947 - Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin

P2, patch with tests and STR for manual QA to verify the fix, looks low risk enought for beta, uplift approved for 71 beta 9, thanks.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f67f28b1f09

Comment 19

[Cristian Fogel [:cfogel]]

Confirmed issue with 72.0a1(2019-10-28) on Ubuntu 18.04.
Fix verified with with :csasca on 72.0a1(2019-11-11) and 71.0b9 on Ubuntu 18.04 and macOS 10.15.