Closed Bug 1431522 Opened 2 years ago Closed 2 years ago

Remove the 'thumb' binding

Categories

(Core :: XUL, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [xbl-in-content])

Attachments

(1 file)

From https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c2: 

> Neil, is it possible we could remove the "thumb" binding at
> https://searchfox.org/mozilla-central/source/toolkit/content/widgets/
> scrollbar.xml#12? It's bound to the "thumb" element at
> https://searchfox.org/mozilla-central/source/toolkit/content/minimal-xul.
> css#91. I'm not sure exactly what `extends="xul:button"` is doing - is this
> something we could delete and/or move into XUL <thumb> directly instead of
> through XBL?

And https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c3: 

> That line causes the element to get a nsButtonBoxFrame instead of nsBoxFrame.
> 
> But you should be able to easily remove the need for a binding by adding a
> mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList
Neil, does the `display: -moz-box !important` on thumb https://dxr.mozilla.org/mozilla-central/source/toolkit/content/minimal-xul.css#92 remove the need to set up the mapping for nsButtonBoxFrame in nsCSSFrameConstructor::FindXULTagData?
Flags: needinfo?(enndeakin)
No, that just gives it a box frame, which is generally redundant since it should already be one. It looks like the display line was added by 248407 to fix a crash.
Flags: needinfo?(enndeakin)
> But you should be able to easily remove the need for a binding by adding a
> mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList

I've looked at nsXBLPrototypeBinding::CheckTagNameWhiteList and it's not clear to me why we would need to whitelist thumb here. That appears to be used for the 'display' or 'extends' attributes on the binding, and I don't see anything that uses display='xul:thumb' or extends='xul:thumb' (https://dxr.mozilla.org/mozilla-central/search?q=display%3D%22xul&redirect=true / https://dxr.mozilla.org/mozilla-central/search?q=extends%3D%22xul&redirect=false).

The only non-tag hit for "xul:thumb" has to do with accesibility roles, which shouldn't be affected by that: https://dxr.mozilla.org/mozilla-central/search?q=xul%3Athumb&redirect=true.  It's a bit odd that the thumb itself doesn't have that role, but doesn't seem like removing the thumb binding would change anything there.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> > But you should be able to easily remove the need for a binding by adding a
> > mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also
> > adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList
> 

Yes, I realize that you don't need to add it in this case. Adding to FindXULTagData should suffice.
I'm not sure if the change to FindXULTagData is right - I just copied what was there for `button`.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Whiteboard: [xbl-in-content]
Comment on attachment 8943979 [details]
Bug 1431522 - Remove the 'thumb' binding;+6102

Somehow the reviewer field didn't get set in mozreview
Attachment #8943979 - Flags: review?(enndeakin)
Comment on attachment 8943979 [details]
Bug 1431522 - Remove the 'thumb' binding;+6102

https://reviewboard.mozilla.org/r/214304/#review229552
Attachment #8943979 - Flags: review?(enndeakin) → review+
Rebased - review flag somehow didn't get carried over but I think mozreview still sees it as an r+ since the land commits button is active.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee37791e04a98f9d7f87f2b435817601be970bd4
I am pretty sure when this lands the scalethumb binding can be removed as well (in bug 1440393).
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> I am pretty sure when this lands the scalethumb binding can be removed as
> well (in bug 1440393).

Yeah - although it looks like we'll also probably need to import scale.css into components.css and remove it out of all <resources> in scale.xml.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a20647d8a887
Remove the 'thumb' binding;r=enndeakin+6102+6102
https://hg.mozilla.org/mozilla-central/rev/a20647d8a887
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.