1590280 - Scroll repeat button icons of menupopup are temporarily displayed as folder icon when the Browser Toolbox is opened

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Alice0775 White]

Attachment: screenshot

+++ This bug was initially created as a clone of Bug #1589089 +++

Though it is worse(Bug 1589089) on Latest Nightly, I found the following bug prior to the regression.

Reproducible: almost 100% if Browser Toolbox was opened

Steps To Reproduce:
1."Enable browser chrome and add-ons debugging toolbars" and "Enable remote debugging"
2. Enable Menu Bar or Bookmarks Toolbar with subfolders and a lot of buukmarks, so that menupopup will be overflowed
3. Open Browser Toolbox (Ctrl+Alt+Shift+I)
4.Click "Bookmarks" in Menu Bar, subfolder etc...

Actual Results:
Repeat button icons of menupopup are temporarily displayed as folder icon.
See attached screen shot.

Regression Window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c420af7f34af85867c4afdec0e32a64dbfbda056&tochange=0257933d9e3104536d2352e559c64f6f42808ee0

Regressed by:
0257933d9e3104536d2352e559c64f6f42808ee0 Alexander Surkov — Bug 1555497 - Remove menupopup binding, r=bgrins

Comment 1

[Alice0775 White]

Attachment: screenshot on Latest Nightly(incle the offending Bug 1555497 and bug 1514926)

Comment 2

[alexander :surkov (:asurkov)]

It turns out that this is caused by list-style-image style on toolbarbutton of an arrowscrollbox of a menupoup, which is inherited from containing menuitem, for example [1]. list-style-image style on that toolbarbutton is overridden by scrollbar.css loaded from global.css from shadow DOM of arrowscrollbox [2]. It seems that scrollbox.css styles are applied lately, what causes folder icons appear temporary on the screen.

Emilio, is it something you could help with?

[1] https://searchfox.org/mozilla-central/source/browser/themes/windows/browser.css#729
[2] https://searchfox.org/mozilla-central/source/toolkit/content/widgets/arrowscrollbox.js#25

Comment 3

[Brian Grinstead [:bgrins]]

Should list-style-image from browser.css be inheriting into shadow contents from the host in the first place?

Comment 4

[Richard Marti (:Paenglab)]

In TB we had a similar issue (bug 1586442). I fixed it through CSS. Maybe you have a better solution.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So did I understand correctly that global.css loads async in the shadow tree, but that it only happens iff the browser toolbox is displayed?

If so I'd need to poke at why, but my guess is that devtools is doing something weird with the stylesheets that is making them not match the cache, or something.

(In reply to Richard Marti (:Paenglab) from comment #4)

In TB we had a similar issue (bug 1586442). I fixed it through CSS. Maybe you have a better solution.

That looks different, right? That seemed to be permanent (that is, just conflicting CSS rules or what not).

(In reply to Brian Grinstead [:bgrins] from comment #3)

Should list-style-image from browser.css be inheriting into shadow contents from the host in the first place?

Yeah list-style-image is inherited, so it's expected.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

ni? just to confirm my undestanding is correct.

If it is, then it's likely not a huge priority as this happens only with the toolbox open, but I could take a look anyway.

Comment 7

[alexander :surkov (:asurkov)]

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

ni? just to confirm my undestanding is correct.

If it is, then it's likely not a huge priority as this happens only with the toolbox open, but I could take a look anyway.

yes, I can reproduce the bug only when the browser toolbox is open, and yeah, it looks that global.css is applied after a period of time after menupopup is shown on the screen, note those all are heuristic observations.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ok, I'll keep this on my radar, but I'll probably look at more important stuff sooner than this :)

Comment 9

[Pascal Chevrel:pascalc]

Fix-optional for 71 as this is not a major problem but if a safe patch materializes, I would evaluate an uplift to beta.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

So this is basically bug 1590513. I posted there a workaround which would fix this and should be pretty safe. But I'm curious to know if there's any other case that doesn't involve the toolbox.

Otherwise we need to audit our usage of <link> in Shadow DOM, specially given the state of XUL layout :)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(Ammended my comment above)

Another thing that would guarantee no FOUC is adopted stylesheets, I guess. That just moves the cache from the XUL prototype cache to the browser JS, but since we don't have the "no observability from clones" requirement when you poke at CSSOM (since the JS code is what intentionally shared them) we'd have no copy-on-write happening...

Thoughts?

Comment 13

[Brian Grinstead [:bgrins]]

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

(Ammended my comment above)

Another thing that would guarantee no FOUC is adopted stylesheets, I guess. That just moves the cache from the XUL prototype cache to the browser JS, but since we don't have the "no observability from clones" requirement when you poke at CSSOM (since the JS code is what intentionally shared them) we'd have no copy-on-write happening...

Thoughts?

What would using adopted stylesheets look like in practice (like for arrowscrollbox)?

Comment 14

[Brian Grinstead [:bgrins]]

Scanning consumers at https://searchfox.org/mozilla-central/search?q=html%3Alink&case=false&regexp=false&path=toolkit%2Fcontent%2Fwidgets

• The ones that load chrome://global/content/widgets.css should really be loading global.css
• <dialog> seems to have a special case where it injects in content styles

If we could have a helper function / getter on the base class that returns a reference to global.css I think we could do this.shadowRoot.prepend(MozXULElement.globalCSS) or similar instead of including it in the parseXULToFragment.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

So something like setting this.shadowRoot.adoptedStyleSheets = [globalCssSheet];, where you'd get globalCssSheet from somewhere like a cache of sorts.

The "cache of sorts" is handwavy, you'd probably want to do something like:

class SheetCache {
  // ...
  async static get(url) {
    if (this._cached[url])
      return this._cached[url];
    let sheet = await new Promise((resolve, reject) => {
      // Do this so we're likely to hit the prototype cache.
      let link = document.createElement("link");
      link.rel = "alternate stylesheet"; // Load it, but don't apply it.
      link.href = url;
      link.onload = () => {
        resolve(link.sheet);
        link.remove();
      };
      link.onerror = () => reject(url); // Or something more descriptive
      document.documentElement.appendChild(link);
    });
    this.cached[url] = sheet;
    return sheet;
}

Makes the potential async-ness more explicit, or you could load all the caches upfront, I guess.

Comment 16

[Brian Grinstead [:bgrins]]

I'd prefer to keep this sync if possible since we run this often in constructors or in relation to other browser js that expects it to be sync. But let me take a closer look later.

Comment 18

[Brian Grinstead [:bgrins]]

There's also Bug 1513285 which had a plan to use the prototype cache for the fragments we pass into parseXULToFragment. Maybe that could be an alternative solution here as long as we could make a mechanism that we are happy with (like, not requiring to move all fragments into new files).

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Grinstead [:bgrins] from comment #18)

There's also Bug 1513285 which had a plan to use the prototype cache for the fragments we pass into parseXULToFragment. Maybe that could be an alternative solution here as long as we could make a mechanism that we are happy with (like, not requiring to move all fragments into new files).

Not sure how that helps? Stylesheets don't load until their <link> or <style> element becomes connected.

Comment 20

[Brian Grinstead [:bgrins]]

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

(In reply to Brian Grinstead [:bgrins] from comment #18)

There's also Bug 1513285 which had a plan to use the prototype cache for the fragments we pass into parseXULToFragment. Maybe that could be an alternative solution here as long as we could make a mechanism that we are happy with (like, not requiring to move all fragments into new files).

Not sure how that helps? Stylesheets don't load until their <link> or <style> element becomes connected.

OK, nevermind then. Did you post the root cause of the problem we are hitting somewhere? I believe I did read it somewhere but I don't see it in this bug.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

The root cause is bug 1590513 comment 1.

Comment 23

[Brian Grinstead [:bgrins]]

Since we don't have adoptedStyleSheets available, would doing something like this work?

let existingGlobalSheet = /* loop through sheets in the document and find the styleSheet. We almost always will have something here. If not we can make it and just let it be async since this is likely only a test it should probably be loading it */;
this.attachShadow();
this.shadowRoot.append(existingGlobalSheet.cloneNode());
this.shadowRoot.append(theRestOfTheFragment);

I'm not sure if the cloned sheet would also have an async load in the Browser Toolbox case.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Not really, each <link> has its own stylesheet. That is, that would work the same way as today, but will still fail with the toolbox open.

Comment 25

[Brian Grinstead [:bgrins]]

So we could do some workarounds on the most obvious issues like this one with forcing the list-style-image to none or by hiding the contents until sheets load. But it sounds like this is a use case for Constructable Sheets (bug 1520690). I haven't heard of this feature until recently, so I have a couple of basic questions:

1. Would that solve our needs for this (approximately Comment 23 but setting this.shadowRoot.adoptedStyleSheets instead of cloning the node)?
2. Is there anything blocking us implementing that feature other than prioritization (spec issues, etc)?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Grinstead [:bgrins] from comment #25)

So we could do some workarounds on the most obvious issues like this one with forcing the list-style-image to none or by hiding the contents until sheets load. But it sounds like this is a use case for Constructable Sheets (bug 1520690). I haven't heard of this feature until recently, so I have a couple of basic questions:

1. Would that solve our needs for this (approximately Comment 23 but setting this.shadowRoot.adoptedStyleSheets instead of cloning the node)?

Yes, if you want global.css changes in the inspector to affect all the shadow roots. We could also fix the inspector so by default doesn't make the sheet uncacheable, too... :)

2. Is there anything blocking us implementing that feature other than prioritization (spec issues, etc)?

Not really, https://github.com/WICG/construct-stylesheets/issues/45 is the only issue that I'd consider a blocker. But we could probably implement the feature and enable it on chrome content without waiting for that, as the solution that won seems to be a decent chunk of work that isn't finished yet :)

Comment 27

[Brian Grinstead [:bgrins]]

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

(In reply to Brian Grinstead [:bgrins] from comment #25)

So we could do some workarounds on the most obvious issues like this one with forcing the list-style-image to none or by hiding the contents until sheets load. But it sounds like this is a use case for Constructable Sheets (bug 1520690). I haven't heard of this feature until recently, so I have a couple of basic questions:

1. Would that solve our needs for this (approximately Comment 23 but setting this.shadowRoot.adoptedStyleSheets instead of cloning the node)?

Yes, if you want global.css changes in the inspector to affect all the shadow roots. We could also fix the inspector so by default doesn't make the sheet uncacheable, too... :)

2. Is there anything blocking us implementing that feature other than prioritization (spec issues, etc)?

Not really, https://github.com/WICG/construct-stylesheets/issues/45 is the only issue that I'd consider a blocker. But we could probably implement the feature and enable it on chrome content without waiting for that, as the solution that won seems to be a decent chunk of work that isn't finished yet :)

Here's what I'm thinking:

(a) fix the inspector so we can keep our current/expected sync behavior (to the extent that it's a relatively easy and upliftable fix to take care of this bug and dupes
(b) implement the new standard which provides a proper solution to this problem and will allow us to stop relying on the sync behavior. Acknowledging that we've been dogfooding Shadow DOM and identified this FOUC as a problem - which we happen to almost always avoid because of special powers, but that web authors don't have this luxury.

Does that make sense to you? Would you or someone on your team be able to take a look at (a) in the shorter term, and then we could separately work on prioritizing (b) in a way that makes sense with relation to existing DOM/CSS work?

Comment 28

[alexander :surkov (:asurkov)]

Do I understand correct that (b) from comment 27 (which is about this.shadowRoot.adoptedStyleSheets new feature) is not something doable in 71 time frame (and perhaps 72 as well)? However this is something that should be fixed next release? If so, then are we looking for a workaround patch similar to one Emilio suggested in bug 1590513 comment #1?

Comment 29

[Brian Grinstead [:bgrins]]

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

Do I understand correct that (b) from comment 27 (which is about this.shadowRoot.adoptedStyleSheets new feature) is not something doable in 71 time frame (and perhaps 72 as well)? However this is something that should be fixed next release? If so, then are we looking for a workaround patch similar to one Emilio suggested in bug 1590513 comment #1?

Emilio could give a more authoritative answer, but yeah I don't expect (b) to be tied to our time frame on this and related bugs. I'd prefer if we could do (a) from Comment 27 in the mean time and avoid adding workrounds to our Custom Elements - especially if when we do get (b) we would then want to remove them anyway.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Does that make sense to you? Would you or someone on your team be able to take a look at (a) in the shorter term, and then we could separately work on prioritizing (b) in a way that makes sense with relation to existing DOM/CSS work?

Sorry for the lag, but no, I don't think my team would be the right one to look at it. I could try... But you would get the async behavior when inspecting stylesheets and such.

I think ideally chrome code shouldn't rely on it, but I plan to look at constructable stylesheets soon-ish anyway.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:bgrins, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 32

[Brian Grinstead [:bgrins]]

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

Does that make sense to you? Would you or someone on your team be able to take a look at (a) in the shorter term, and then we could separately work on prioritizing (b) in a way that makes sense with relation to existing DOM/CSS work?

Sorry for the lag, but no, I don't think my team would be the right one to look at it. I could try... But you would get the async behavior when inspecting stylesheets and such.

I think ideally chrome code shouldn't rely on it, but I plan to look at constructable stylesheets soon-ish anyway.

Why do you think chrome code shouldn't rely on constructable stylesheets?

Comment 33

[Emilio Cobos Álvarez (:emilio)]

I meant that it shouldn't rely on <link> behaving synchronously. Constructable stylesheets would be fine.

Comment 34

[Brian Grinstead [:bgrins]]

Since this only happens when the Browser Toolbox is opened, it's not going to be seen by nearly any user. We should still fix this though, so I'm marking as P2 since it's surfaced an issue where we are relying on a sort of fragile style cache. Depending on the timeline for constructable stylesheets this could probably wait for that.

Comment 35

[Brian Grinstead [:bgrins]]

Tentatively blocking on the constructable stylesheet feature (bug 1520690), but we might want to make a frontend workaround for this specific thing sooner.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1590280 - Allow to use the sheet cache to avoid parsing as long as CSSOM hasn't mutated the stylesheet. r=bzbarsky,heycam

We need to ensure we have a unique inner so that ruleLists and such have the
right pointer identity (we could do better, really, but it's harder).

But as long as the CSSOM hasn't modified them there should be no reason not to
use the cache. We can do a deep clone synchronously instead of refetching /
reparsing.

This is important because, as of right now, just using the inspector makes the
stylesheets unique, which is unfortunate.

We'll still have the modified rule bit for sheets with @import, because our
notification system for @import is silly, and on parents of imported sheets.

Fixing those are future improvements, but I see no reason not to land this.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dee20d080337
Allow to use the sheet cache to avoid parsing as long as CSSOM hasn't mutated the stylesheet. r=heycam

Comment 38

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/dee20d080337

Comment 39

[Boris Zbarsky [:bzbarsky]]

Emilio, can you please take a look at https://phabricator.services.mozilla.com/D55163#1694428 ?

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Replied there.

Comment 41

[Julien Cristau [:jcristau]]

Feel free to request uplift to beta if you think it's worth it.

Comment 42

[Brian Grinstead [:bgrins]]

(In reply to Julien Cristau [:jcristau] from comment #41)

Feel free to request uplift to beta if you think it's worth it.

I don't think it is. Most Browser Toolbox users will be on Nightly / local builds.

Comment 43

[Maria Berlinger [:mberlinger], Services QA]

Hello,

Unfortunately I wasn’t able to reproduce this issue using Firefox 72.0a1 (2019-10-21) by following the STR from Comment 0.

Alice, could you please verify it on the latest beta build? (https://archive.mozilla.org/pub/firefox/candidates/73.0b12-candidates/build1/win64/en-US/)

Comment 44

[Alice0775 White]

Reproduced on 73.0a1(20191203094830) Windows10.
And verified fixed on 73.0b11(20200128001646) Windows10 and Nightly74.0a1(20200131013147) Windows10.

Comment 45

[Maria Berlinger [:mberlinger], Services QA]

Thanks Alice,
Based on the above comment dropping the qe flag.