Status

()

enhancement
P5
normal
REOPENED
3 months ago
an hour ago

People

(Reporter: timdream, Assigned: surkov)

Tracking

(Blocks 2 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 affected)

Details

(Whiteboard: [xbl-available])

Attachments

(5 attachments, 1 obsolete attachment)

The following bindings can be converted to custom elements by prepending/appending elements when CE connects.

They would need the basecontrol mixin to be landed in bug 1518932.

  • tab
  • tabbrowser-tab

Specifically for tabbrowser-tab we could move the complexity to its only user if applicable.

Watch out for performance issue with a lot of tabs though.

raw note:

tab
Has <content> + <children>, but it’s only prepending and appending. Used lightly (devtools and maybe one other place).
tabbrowser-tab
Coupled with tab. Could be used as a tab with [is].
Look into performance + sessionrestore with a lot of tabs

Updated

3 months ago
Priority: -- → P5
(Assignee)

Updated

16 days ago
Assignee: nobody → surkov.alexander
(Assignee)

Comment 4

16 days ago

I've run into a tooltip over tab icon problem (D26602 patch). If I make tab's content explicit, then document.tooltipNode is tab's icon the mouse is over at, which is expected I think. If I put tab's content into shadow DOM, then document.tooltipNode is parent tabs element for unknown reason.

Emilio, do you have ideas on this issue?

Flags: needinfo?(emilio)

Without having looked at it for too long, seems like ultimately the result of that operation comes from:

https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/layout/xul/nsMenuFrame.cpp#628

Is there any chance that popup is using the anchor attribute, and that either the popup or the target of the anchor are now in a Shadow tree?

Flags: needinfo?(emilio)
(Assignee)

Comment 6

15 days ago

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

Without having looked at it for too long, seems like ultimately the result of that operation comes from:

https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/layout/xul/nsMenuFrame.cpp#628

Is there any chance that popup is using the anchor attribute,

unlikely, I don't see setAttribute for @anchor in code, https://searchfox.org/mozilla-central/search?q=setAttribute%28%22anchor&path= or relevant anchor="" in markup, https://searchfox.org/mozilla-central/search?q=+anchor%3D&path=

and that either the popup or the target of the anchor are now in a Shadow tree?

popup is an explicit node living in browser.xul, aka tooltip#tabbrowser-tab-tooltip, https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#585

(Assignee)

Comment 7

15 days ago

It appears that when the test waves over <image.tab-icon-sound> icon located in tab's shadow DOM [1], then mouse move event target is <tabs id='tabbrowser-tabs'> element [2]. Does it look like eventing bug?

[1] https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/browser_audioTabIcon.js#61
[2] https://searchfox.org/mozilla-central/source/layout/xul/nsXULTooltipListener.cpp#265

Flags: needinfo?(emilio)

Looks like the XUL tooltip code should use GetComposedTarget / GetOriginalTarget instead of GetTarget. Sorry, should've thought of that. See bug 1489440 for a similar issue.

Flags: needinfo?(emilio)
(Assignee)

Comment 9

15 days ago

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

Looks like the XUL tooltip code should use GetComposedTarget / GetOriginalTarget instead of GetTarget. Sorry, should've thought of that. See bug 1489440 for a similar issue.

XUL tooltip tries GetComposedTarget (which is <scrollbox> in this case), and then if doesn't have GetContainingShadow (it doesn't), then it falls back to GetTarget(), which is <tabs>, see [1].

Honestly I'm not sure how the tooltip logic should be modified to make tooltips work. Also having <tabs> as an event target of mouse move over tab's shadow DOM icon is confusing, because <tab> is an explicit child of <tabs>. Either I don't understand what event target is or there's some eventing bug :)

[1] https://searchfox.org/mozilla-central/source/layout/xul/nsXULTooltipListener.cpp#178

(Assignee)

Comment 10

14 days ago

ni? from Emilio on comment #9

Flags: needinfo?(emilio)

Honestly I'm not sure how the tooltip logic should be modified to make tooltips work. Also having <tabs> as an event target of mouse move over tab's shadow DOM icon is confusing, because <tab> is an explicit child of <tabs>. Either I don't understand what event target is or there's some eventing bug :)

Event.target is retargeted, so if you're observing it from outside the shadow tree, like the tooltip listener does, then the topmost shadow host is what you see.

Can you give me some concrete STR of a tooltip not showing up or a test failing or something like with your patch that that I could debug?

Flags: needinfo?(emilio)
(Assignee)

Comment 12

12 days ago

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

Honestly I'm not sure how the tooltip logic should be modified to make tooltips work. Also having <tabs> as an event target of mouse move over tab's shadow DOM icon is confusing, because <tab> is an explicit child of <tabs>. Either I don't understand what event target is or there's some eventing bug :)

Event.target is retargeted, so if you're observing it from outside the shadow tree, like the tooltip listener does, then the topmost shadow host is what you see.

Can you give me some concrete STR of a tooltip not showing up or a test failing or something like with your patch that that I could debug?

I have updated the patches, now it is a version where shadow content is used. I can see a tooltip issue when running browser/base/content/test/tabs/browser_audioTabIcon.js test.

Flags: needinfo?(emilio)

I took a look at this. I can indeed see the issue. The tooltipNode is the tabs instead of the tab, but that's because you're hitting the arrowscrollbox behind the tab bar, not a tab:

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/content/tabbrowser.xml#20

In general, this patch seems to need a bunch more work. There's a lot of CSS that applies to tabs right now that doesn't apply if you stash the tabs into a shadow root.

You should move all the tab-specific CSS into the shadow root. Probably using :host(..) rules to migrate the tab selectors, and normal rules for the rest of the content inside the tab.

Flags: needinfo?(emilio)
(Assignee)

Comment 14

7 days ago

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

I took a look at this. I can indeed see the issue. The tooltipNode is the tabs instead of the tab, but that's because you're hitting the arrowscrollbox behind the tab bar, not a tab:

it's weird, because the test moves the mouse over a tab icon, and event target shouldn't depend on applied styles

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/content/tabbrowser.xml#20

In general, this patch seems to need a bunch more work. There's a lot of CSS that applies to tabs right now that doesn't apply if you stash the tabs into a shadow root.

You should move all the tab-specific CSS into the shadow root. Probably using :host(..) rules to migrate the tab selectors, and normal rules for the rest of the content inside the tab.

you're right, it requires some effort to adjust cross shadow DOM styling like:

#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background[selected="true"]:-moz-lwtheme {

might be worth to give a try to explicit content again.

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

you're right, it requires some effort to adjust cross shadow DOM styling like:

#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background[selected="true"]:-moz-lwtheme {

This isn't possible right now. CSS Shadow Parts will let us do that (to be implemented for chrome in https://bugzilla.mozilla.org/show_bug.cgi?id=1505489).

Comment 18

2 days ago
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc479e4e17f2
make sure document.l10n is initialized before triggering conext menu over a tab in browser_ext_menus_activeTab.js test r=Gijs

Comment 19

2 days ago
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8858cc62fe60
make xul/mac-tab-toolbar.xul test running under chrome privileges r=bgrins

Comment 20

a day ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a day ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
(Assignee)

Updated

a day ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

18 hours ago

so far there are two performance failing tests [1]:

  • performance/browser_appmenu.js [2]:
    TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu.js | unexpected changed rect: ({x1:0, x2:223, y1:0, y2:32, w:224, h:33}), window width: 1280 -
    TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_appmenu.js | should have 0 unknown flickering areas - Got 1, expected 0
  • performance/browser_startup_flicker.js [3]
    TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_flicker.js | unexpected changed rect: ({x1:0, x2:223, y1:0, y2:32, w:224, h:33}), window width: 1280 -
  • TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_flicker.js | should have 0 unknown flickering areas - Got 1, expected 0

Any ideas/hints what these failures are about and how to debug these tests?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f76f45b9de59f78c7b2c8dcf069d646e7e2c2f4c&selectedJob=242139536
[2] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=242139536&repo=try&lineNumber=2323
[3] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=242140703&repo=try&lineNumber=2991

Curious if you drop delayConnectedCallback if the errors change / go away (https://hg.mozilla.org/try/rev/fe8a60d5f24c2a61148cad73948666337e8e08d7#l3.769). That early return is to avoid XBL construction before layout, but just glancing at the content in this.fragment none of those seem to have XBL attached so perhaps we could drop that if it helps.

(Assignee)

Comment 23

2 hours ago

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

Curious if you drop delayConnectedCallback if the errors change / go away (https://hg.mozilla.org/try/rev/fe8a60d5f24c2a61148cad73948666337e8e08d7#l3.769). That early return is to avoid XBL construction before layout, but just glancing at the content in this.fragment none of those seem to have XBL attached so perhaps we could drop that if it helps.

it was a remedy indeed for performance/browser_startup_flicker.js

performance/browser_appmenu.js seems feel well now with a couple more changes, https://treeherder.mozilla.org/#/jobs?repo=try&revision=07079539fa3fdf05f5ca223cf9be7e20e1c542cc

Attachment #9056698 - Attachment is obsolete: true
Attachment #9056697 - Attachment description: Bug 1519514 - hg copy tabbrowser.xml to tabbrowser-tabs.js to preserve history for tab binding conversion to a Custom Element, r=bgrins → Bug 1519514 - hg copy tabbrowser.xml to tabbrowser-tab.js to preserve history for tab binding conversion to a Custom Element, r=bgrins
You need to log in before you can comment on or make changes to this bug.