Closed Bug 1590280 Opened 1 year ago Closed 1 year ago

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

Categories

(Toolkit :: XUL Widgets, defect, P2)

71 Branch
x86_64
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla73
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- verified
firefox74 --- verified

People

(Reporter: alice0775, Assigned: emilio)

References

(Depends on 1 open bug, Blocks 1 open bug, Regression)

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

Attached image 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

Summary: Repeat button icons of menupopup are temporarily displayed as folder icon → Scroll repeat button icons of menupopup are temporarily displayed as folder icon

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

Flags: needinfo?(emilio)

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

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

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.

Flags: needinfo?(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.

Flags: needinfo?(surkov.alexander)

(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.

Flags: needinfo?(surkov.alexander)

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

Flags: needinfo?(emilio)

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

Duplicate of this bug: 1590513

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 :)

Flags: needinfo?(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?

Flags: needinfo?(bgrinstead)

(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)?

Flags: needinfo?(bgrinstead) → needinfo?(emilio)

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.

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.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(bgrinstead)
Duplicate of this bug: 1589089

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).

(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.

(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.

The root cause is bug 1590513 comment 1.

See Also: → 1591947
See Also: 1591947
Duplicate of this bug: 1591947

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.

Flags: needinfo?(bgrinstead) → needinfo?(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.

Flags: needinfo?(emilio)

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)?
Flags: needinfo?(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... :)

  1. 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 :)

Flags: needinfo?(emilio)

(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... :)

  1. 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?

Flags: needinfo?(emilio)

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?

(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.

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.

Flags: needinfo?(emilio)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(bgrinstead)
Summary: Scroll repeat button icons of menupopup are temporarily displayed as folder icon → Scroll repeat button icons of menupopup are temporarily displayed as folder icon when the Browser Toolbox is opened

(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?

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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.

Flags: needinfo?(bgrinstead)
Priority: -- → P2

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

Assignee: nobody → emilio

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.

Blocks: 1600155
No longer depends on: 1589089
See Also: → 1589089
Pushed by ealvarez@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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

Flags: needinfo?(emilio)

Replied there.

Flags: needinfo?(emilio)

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

(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.

Flags: qe-verify+

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/)

Flags: needinfo?(alice0775)

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

Status: RESOLVED → VERIFIED
Flags: needinfo?(alice0775)

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

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.