Closed Bug 1448213 Opened 7 years ago Closed 6 years ago

Migrate label-control binding to a Custom Element

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: timdream, Assigned: bgrins)

References

Details

(Whiteboard: [overhead:?])

Attachments

(1 file, 1 obsolete file)

Bug 1448213 - Migrate label-control to a Custom Element
46 bytes, text/x-phabricator-request
Details | Review 
text-base is currently bound to <description> and text-label to <label>. They are both pretty small. The only problem here is that <description> will get extra `accessKey` and `control` properties with this merge.

On the other hand, <description> can be a clean target to migrate to custom element (no need to update markup, no in-content usage, no inheritance), so doing this could be an immature optimization.

:bgrins, what do you think? cf your work at bug 1411707.
Flags: needinfo?(bgrinstead)
Assignee: timdream → nobody
Status: ASSIGNED → NEW
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)
> text-base is currently bound to <description> and text-label to <label>.
> They are both pretty small. The only problem here is that <description> will
> get extra `accessKey` and `control` properties with this merge.
> 
> On the other hand, <description> can be a clean target to migrate to custom
> element (no need to update markup, no in-content usage, no inheritance), so
> doing this could be an immature optimization.
> 
> :bgrins, what do you think? cf your work at bug 1411707.

I'd be happy to add a couple of extra properties on <description> if it meant that we could merge them together and remove the binding.

I assume in this case we would copy the impl from text-label into text-base and then have text-base implement nsIDOMXULLabelElement. Then bind both <label> and <description> to text-base, delete text-label and nsIDOMXULDescriptionElement? The only thing I see that might change behavior in that case is some code accessibility code and a reference in nsTextBoxFrame: https://searchfox.org/mozilla-central/search?q=nsIDOMXULLabelElement&path=.

Forwarding needinfo to Neil to get another opinion, and also to see if the stuff he's looking at in Bug 1446961 might help for removing the [implements].
Flags: needinfo?(bgrinstead) → needinfo?(enndeakin)
Oh, and regarding <label> as a CE - if we can move any frame / accessibility stuff to be keyed on the tag name instead then I think we should be able to make a single "label" custom element that covers both "label-control" (attached to `label[control], label.radio-label, label.checkbox-label, label.toolbarbutton-multiline-text`) and "text-link" (attached to `label.text-link, label[onclick]`).

We'll need to merge all the functionality into a single CE (since we can only register one per tag name) but we can make the individual properties / methods in those two bindings conditional based on the various classes / attributes from the CSS selectors. It looks like there's some strangeness in text-label.control property where it switches the binding at runtime to label-control but luckily I don't see that being used: https://searchfox.org/mozilla-central/search?q=.control+%3D&case=false&regexp=false&path=.
(In reply to Brian Grinstead [:bgrins] from comment #1)
> I'd be happy to add a couple of extra properties on <description> if it
> meant that we could merge them together and remove the binding.
> 
> I assume in this case we would copy the impl from text-label into text-base
> and then have text-base implement nsIDOMXULLabelElement. Then bind both
> <label> and <description> to text-base, delete text-label and
> nsIDOMXULDescriptionElement? The only thing I see that might change behavior
> in that case is some code accessibility code and a reference in
> nsTextBoxFrame:
> https://searchfox.org/mozilla-central/search?q=nsIDOMXULLabelElement&path=.
> 
> Forwarding needinfo to Neil to get another opinion, and also to see if the
> stuff he's looking at in Bug 1446961 might help for removing the
> [implements].

I think we could keep nsIDOMXULDescriptionElement (and the behavior of binary unchanged). Just have the new binding (probably should be named simply "text") implements both interfaces.

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Oh, and regarding <label> as a CE - if we can move any frame / accessibility
> stuff to be keyed on the tag name instead then I think we should be able to
> make a single "label" custom element that covers both "label-control"
> (attached to `label[control], label.radio-label, label.checkbox-label,
> label.toolbarbutton-multiline-text`) and "text-link" (attached to
> `label.text-link, label[onclick]`).
> 
> We'll need to merge all the functionality into a single CE (since we can
> only register one per tag name) but we can make the individual properties /
> methods in those two bindings conditional based on the various classes /
> attributes from the CSS selectors. It looks like there's some strangeness in
> text-label.control property where it switches the binding at runtime to
> label-control but luckily I don't see that being used:
> https://searchfox.org/mozilla-central/search?q=.
> control+%3D&case=false&regexp=false&path=.

That's a lot of features to merge into one <label> CE. I was thinking about bulk remaining of all other usages into new tag names[1] and keep the inheritance b/t them, but that might be riskier. Any particular reason why that's a route we should avoid?

If so, we will need to go through the list again to think about what to merge before/while we convert bindings to CEs.

[1] I was thinking about

- FirefoxText, register to <label> and <description>
- FirefoxControlLabel, register to <label-control>
- FirefoxLinkLabel, register to <label-link>

(and patch XULMap.h for it to recognize these new tag names)
(In reply to Brian Grinstead [:bgrins] from comment #1)
> I assume in this case we would copy the impl from text-label into text-base
> and then have text-base implement nsIDOMXULLabelElement. Then bind both
> <label> and <description> to text-base, delete text-label and
> nsIDOMXULDescriptionElement? The only thing I see that might change behavior
> in that case is some code accessibility code and a reference in
> nsTextBoxFrame:
> https://searchfox.org/mozilla-central/search?q=nsIDOMXULLabelElement&path=.

PS If we delete nsIDOMXULDescriptionElement, it should not affect nsTextBoxFrame, given it uses nsIDOMXULLabelElement.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> PS If we delete nsIDOMXULDescriptionElement, it should not affect
> nsTextBoxFrame, given it uses nsIDOMXULLabelElement.

Oh, right. In this case as long as we are OK including the extra properties on <desccription> I don't see a reason to keep nsIDOMXULDescriptionElement around (and have the binding implement both two interfaces). The binding should be able to implement just the label interface (possibly renamed to be more generic), yeah?
Brian am I had a chat about this and he thinks for this specific case, it's should be fine to merge the implementations into one binding (to be converted to one custom element register to <label>/<description>).

We would need to make some plans for other tag names though but this can work as a mini test drive.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Component: XBL → XUL Widgets
Product: Core → Toolkit
Comment on attachment 8962257 [details]
Bug 1448213 - Merge text-base, text-label, label-control, text-link bindings into the text binding

https://reviewboard.mozilla.org/r/231122/#review236648

::: toolkit/content/minimal-xul.css
(Diff revision 3)
> -  -moz-binding: url("chrome://global/content/bindings/text.xml#text-label");
> -}
> -
> -label.text-link, label[onclick] {
> -  -moz-binding: url("chrome://global/content/bindings/text.xml#text-link");
> -  -moz-user-focus: normal;

Looks like the `-moz-user-focus` rule got lost with the change

::: toolkit/content/widgets/text.xml:12
(Diff revision 3)
>  <bindings id="textBindings"
>     xmlns="http://www.mozilla.org/xbl"
> -   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> -   xmlns:html="http://www.w3.org/1999/xhtml">
> +   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <!-- bound to <label>s and <description>s -->
> +  <binding id="text">

I think this'll need the <content> from label-control
Comment on attachment 8962257 [details]
Bug 1448213 - Merge text-base, text-label, label-control, text-link bindings into the text binding

https://reviewboard.mozilla.org/r/231122/#review236658

::: toolkit/content/widgets/text.xml:12
(Diff revision 3)
>  <bindings id="textBindings"
>     xmlns="http://www.mozilla.org/xbl"
> -   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> -   xmlns:html="http://www.w3.org/1999/xhtml">
> +   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <!-- bound to <label>s and <description>s -->
> +  <binding id="text">

<html:span> element is added in JS from the constructor. Using <content> will result redundant element being created from non-label-controls. I don't think that's a good idea...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> Comment on attachment 8962257 [details]
> Bug 1448213 - Merge text-base, text-label, label-control, text-link bindings
> into the text binding
> 
> https://reviewboard.mozilla.org/r/231122/#review236658
> 
> ::: toolkit/content/widgets/text.xml:12
> (Diff revision 3)
> >  <bindings id="textBindings"
> >     xmlns="http://www.mozilla.org/xbl"
> > -   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > -   xmlns:html="http://www.w3.org/1999/xhtml">
> > +   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> > +
> > +  <!-- bound to <label>s and <description>s -->
> > +  <binding id="text">
> 
> <html:span> element is added in JS from the constructor. Using <content>
> will result redundant element being created from non-label-controls. I don't
> think that's a good idea...

Got it - missed that it was being added in the constructor. Although it looks like it's appending the span as normal content so formatAccessKey shouldn't find it when doing getAnonymousElementByAttribute. I also don't know if it'll have any side effects, but if it works that does seem better than adding <content> to every label/description.
(In reply to Brian Grinstead [:bgrins] from comment #14)
> Got it - missed that it was being added in the constructor. Although it
> looks like it's appending the span as normal content so formatAccessKey
> shouldn't find it when doing getAnonymousElementByAttribute. I also don't
> know if it'll have any side effects, but if it works that does seem better
> than adding <content> to every label/description.

You were right — it turned out there isn't a way to add an anonymous node from the JS. Not yet until we ship Shadow DOM.

I am restoring the <content> part here which would add <html:span/> to every label/description.
There are a few test failure related to event handling. Apparently addEventListener() etc in the patch doesn't equivalent to the original <handler>s in some way. I would need to find out the difference there.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #17)
> There are a few test failure related to event handling. Apparently
> addEventListener() etc in the patch doesn't equivalent to the original
> <handler>s in some way. I would need to find out the difference there.

I didn't expect that - I didn't see anything like that when migrating tabbrowser away from XBL and to a JS module that called addEventListener on the node. But it does look like we may use more <handler> options in these bindings.
Flags: needinfo?(enndeakin)
For the test failure on dom/base/test/test_postMessages.html, it fails because SpecialPowers.pushPrefEnv() wasn't picked up in the test. Strangely, the change that could cause this to happen is the existence of <constructor> in the text-label binding. Will dig deeper from here.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19)
> For the test failure on dom/base/test/test_postMessages.html, it fails
> because SpecialPowers.pushPrefEnv() wasn't picked up in the test. Strangely,
> the change that could cause this to happen is the existence of <constructor>
> in the text-label binding. Will dig deeper from here.

BTW, I suspect this will go away with bug 1446830 since the label lives in <input type=file>? Noted that even with that there is still more test failure in the patch here.
See Also: → 1446830
Priority: -- → P5
I'm thinking of an alternate approach here - based on https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c34 with some more notes:

I believe we could implement text-base, text-label, and label-control bindings in C++. We would create a new XUL element subclass (XULTextElement), which would have all the properties of nsIDOMXULDescriptionElement and nsIDOMXULLabelElement.

A few notes:

1) The couple of Qis into nsIDOMXULLabelElement could change into a tag name check plus cast into the subclass.
2) I believe (although would like to confirm with Masayuki) that the only reason we need to implement formatAccessKey in JS is so that we can call it when `control` changes: https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/content/widgets/text.xml#256. Because it seems like the access key rendering is also handled in C++ when the frame reflows: https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/layout/xul/nsTextBoxFrame.cpp#836. So if we took over handling the [control] change in C++ then we may be able to directly call nsTextBoxFrame::UpdateAccessTitle and drop the duplicated JS implementation.
Flags: needinfo?(masayuki)
(Sorry for the delay to reply.)

I don't remember the detail around XUL label/accesskey display, though.  Additionally, I don't know the reason why formatAccessKey is implemented with JS. However, there were some reasons to use JS better than C++ around 2000. One was easier and faster to hack. In these days, build speed of Firefox binary is pretty faster. IIRC, it took 2x or more hours than now. Therefore, C++ was not used around UI. Another one was, XPCOM string API was really cheap.  E.g., no regex API. Now, we think runtime performance is really important. So, if re-implementing with C++ makes sense, I think that it's not a problem. (But please note that I don't know whether XUL module owner thinks it's reasonable.)
Flags: needinfo?(masayuki)
Depends on: 1487313, 1488116
Depends on: 1488620
(In reply to Brian Grinstead [:bgrins] from comment #21)
> I'm thinking of an alternate approach here - based on
> https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c34 with some more
> notes:
> 
> I believe we could implement text-base, text-label, and label-control
> bindings in C++. We would create a new XUL element subclass
> (XULTextElement), which would have all the properties of
> nsIDOMXULDescriptionElement and nsIDOMXULLabelElement.

I have removed nsIDOMXULDescriptionElement, and about to remove nsIDOMXULLabelElement interface, thus the bindings don't have |implements| part. Is it beneficial to move to c++ implementation, rather than replace the bindings by custom elements? JS to JS should be easier.
(In reply to alexander :surkov (:asurkov) from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > I'm thinking of an alternate approach here - based on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c34 with some more
> > notes:
> > 
> > I believe we could implement text-base, text-label, and label-control
> > bindings in C++. We would create a new XUL element subclass
> > (XULTextElement), which would have all the properties of
> > nsIDOMXULDescriptionElement and nsIDOMXULLabelElement.
> 
> I have removed nsIDOMXULDescriptionElement, and about to remove
> nsIDOMXULLabelElement interface, thus the bindings don't have |implements|
> part. Is it beneficial to move to c++ implementation, rather than replace
> the bindings by custom elements? JS to JS should be easier.

Yes, converting it to C++ would help, since we use a XUL label inside the filepicker UI (which is in-content and so we don't want to have to register Custom Elements for that case). We do want to move that away from using XUL label, but that's blocked on implementing some kind of a middle crop for that case (see https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c33).

We could instead put effort into implementing that middle crop, but I do think moving this to C++ should have a couple of other benefits:
1) Handling at least the most simple cases (text-base for <description> and text-label for bare <label> tags) should help with perf since these are very common elements. This is also the minimum to unblock the filepicker UI issue.
2) If we could handle the label-control case in there as well that could allow de-duplication of the accessKey formatting logic (as per Comments 21 and 22).

So assuming that moving it to C++ doesn't make an eventual transition to HTML elements substantially harder (I don't think it does, since HTML elements have their own accesskey handling), I think it makes sense to at least investigate.
(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to alexander :surkov (:asurkov) from comment #23)
> > (In reply to Brian Grinstead [:bgrins] from comment #21)
> > > I'm thinking of an alternate approach here - based on
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c34 with some more
> > > notes:
> > > 
> > > I believe we could implement text-base, text-label, and label-control
> > > bindings in C++. We would create a new XUL element subclass
> > > (XULTextElement), which would have all the properties of
> > > nsIDOMXULDescriptionElement and nsIDOMXULLabelElement.
> > 
> > I have removed nsIDOMXULDescriptionElement, and about to remove
> > nsIDOMXULLabelElement interface, thus the bindings don't have |implements|
> > part. Is it beneficial to move to c++ implementation, rather than replace
> > the bindings by custom elements? JS to JS should be easier.
> 
> Yes, converting it to C++ would help, since we use a XUL label inside the
> filepicker UI (which is in-content and so we don't want to have to register
> Custom Elements for that case). We do want to move that away from using XUL
> label, but that's blocked on implementing some kind of a middle crop for
> that case (see https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c33).
> 
> We could instead put effort into implementing that middle crop, but I do
> think moving this to C++ should have a couple of other benefits:
> 1) Handling at least the most simple cases (text-base for <description> and
> text-label for bare <label> tags) should help with perf since these are very
> common elements. This is also the minimum to unblock the filepicker UI issue.
> 2) If we could handle the label-control case in there as well that could
> allow de-duplication of the accessKey formatting logic (as per Comments 21
> and 22).
> 
> So assuming that moving it to C++ doesn't make an eventual transition to
> HTML elements substantially harder (I don't think it does, since HTML
> elements have their own accesskey handling), I think it makes sense to at
> least investigate.

Also, moving to C++ has probable memory benefits, as it won’t require using JavaScript in order to handle the label implementation.
Whiteboard: [overhead:?]
(In reply to Brian Grinstead [:bgrins] from comment #24)

> Yes, converting it to C++ would help, since we use a XUL label inside the
> filepicker UI (which is in-content and so we don't want to have to register
> Custom Elements for that case). We do want to move that away from using XUL
> label, but that's blocked on implementing some kind of a middle crop for
> that case (see https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c33).
> 
> We could instead put effort into implementing that middle crop, but I do
> think moving this to C++ should have a couple of other benefits:
> 1) Handling at least the most simple cases (text-base for <description> and
> text-label for bare <label> tags) should help with perf since these are very
> common elements. This is also the minimum to unblock the filepicker UI issue.
> 2) If we could handle the label-control case in there as well that could
> allow de-duplication of the accessKey formatting logic (as per Comments 21
> and 22).
> 
> So assuming that moving it to C++ doesn't make an eventual transition to
> HTML elements substantially harder (I don't think it does, since HTML
> elements have their own accesskey handling), I think it makes sense to at
> least investigate.

Filepicker UI doesn't rely on anything of XBL-bindings, i.e. it operates on DOM attributes, which are implemented by nsTextBoxFrame I think. It means that we don't have to register custom elements to keep it working.

C++ transformation is not trivial not only because c++ is more bulky than js, but also because it should take time to figure out how to merge nsTextBoxFrame code with XBL bindings for example, or because xul:label@ccesskey and other xul controls bindings depend on each other, which means we will need to implement nsIDOMXULLabel interface for labels in c++ as well. If we will keep the implementations in JS, then I think we can kill this interface.

I understand the perf/memory concern. However are custom elements worse than XBL bindings in memory/pref? because if no, then we probably shouldn't be too concerned, since we don't regress anything. XUL description and no-accesskey XUL labels bindings are tiny wrappers providing a bunch of properties to access DOM attributes. I guess, we could move those into c++ and restore nsIDOMXULDescription/Label interfaces (or provide a new, the common one?), but not sure whether it's worth it, especially taking into account XUL elements will be eventually removed anyways.
(In reply to alexander :surkov (:asurkov) from comment #26)
> > So assuming that moving it to C++ doesn't make an eventual transition to
> > HTML elements substantially harder (I don't think it does, since HTML
> > elements have their own accesskey handling), I think it makes sense to at
> > least investigate.
> 
> Filepicker UI doesn't rely on anything of XBL-bindings, i.e. it operates on
> DOM attributes, which are implemented by nsTextBoxFrame I think. It means
> that we don't have to register custom elements to keep it working.

So just to make sure we are on the same page: inside of the NAC in the filepicker we create a XUL <label> (https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/layout/forms/nsFileControlFrame.cpp#148), which then gets the XBL binding attached as in-content via minimal-xul.css (https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/toolkit/content/minimal-xul.css#58).

I think what you are saying is that we could actually already move those rules out of minimal-xul.css (Bug 1446831) once Bug 1488620 lands and we no longer implement nsIDOMXULLabelElement. Is that right?
(In reply to Brian Grinstead [:bgrins] from comment #27)

> So just to make sure we are on the same page: inside of the NAC in the
> filepicker we create a XUL <label>
> (https://searchfox.org/mozilla-central/rev/
> 9e7995b3c384945740985107a4de601b27904718/layout/forms/nsFileControlFrame.
> cpp#148), which then gets the XBL binding attached as in-content via
> minimal-xul.css
> (https://searchfox.org/mozilla-central/rev/
> 9e7995b3c384945740985107a4de601b27904718/toolkit/content/minimal-xul.css#58).
> 
> I think what you are saying is that we could actually already move those
> rules out of minimal-xul.css (Bug 1446831)

I think yes (assuming the filepicker is a single consumer).

> once Bug 1488620 lands and we no
> longer implement nsIDOMXULLabelElement. Is that right?

this one shouldn't be a blocking, so I think we can try to wipe the XUL label/description bindings out of minimal-xul.css right now (for sure if I read code right and don't miss anything).
(In reply to alexander :surkov (:asurkov) from comment #28)
> (In reply to Brian Grinstead [:bgrins] from comment #27)
> 
> > So just to make sure we are on the same page: inside of the NAC in the
> > filepicker we create a XUL <label>
> > (https://searchfox.org/mozilla-central/rev/
> > 9e7995b3c384945740985107a4de601b27904718/layout/forms/nsFileControlFrame.
> > cpp#148), which then gets the XBL binding attached as in-content via
> > minimal-xul.css
> > (https://searchfox.org/mozilla-central/rev/
> > 9e7995b3c384945740985107a4de601b27904718/toolkit/content/minimal-xul.css#58).
> > 
> > I think what you are saying is that we could actually already move those
> > rules out of minimal-xul.css (Bug 1446831)
> 
> I think yes (assuming the filepicker is a single consumer).
> 
> > once Bug 1488620 lands and we no
> > longer implement nsIDOMXULLabelElement. Is that right?
> 
> this one shouldn't be a blocking, so I think we can try to wipe the XUL
> label/description bindings out of minimal-xul.css right now (for sure if I
> read code right and don't miss anything).

OK. I'd like to read through Comment 26 later and think more about the C++ vs Custom Element question. In the meantime if you could take a look at Bug 1446831 to see if that's viable it would be a great start (both for the content process memory win and also for removing the in-content XBL).
Attachment #8962257 - Attachment is obsolete: true
Assignee: timdream → bgrinstead
Depends on: 1494167
Depends on: 1494230
After bug 1494230 (kill #text-base binding) is fixed, we actually can remove #text-label binding as well. However, we'd need to find a new host for accessKey/control attributes. I'm curious would be ok to put those into XULTextElement interface and thus get shared with description element. XUL:description will receive those properties which is weird, but it looks harmless. Thoughts?
(In reply to alexander :surkov (:asurkov) from comment #31)
> After bug 1494230 (kill #text-base binding) is fixed, we actually can remove
> #text-label binding as well. However, we'd need to find a new host for
> accessKey/control attributes. I'm curious would be ok to put those into
> XULTextElement interface and thus get shared with description element.
> XUL:description will receive those properties which is weird, but it looks
> harmless. Thoughts?

It seems harmless to add them to XULTextElement, and more consistent than making <description> a bare XULTextElement and <label> a Custom Element.

There's some trickery with the control property attribute in which it causes the label-control binding to get attached to an existing element. This is not a thing that Custom Elements can do - it's either a customized built-in i.e. `<label is="label-control">` or not.

However, AFAICT we don't ever use .control [0] so perhaps that could (and should) be removed. There's one hit for `setAttribute("control"` [1] but that's just setting up a brand new element so that could be updated to create it as a CE doing something like document.createXULElement("label", { is: "label-control" })

[0]: https://searchfox.org/mozilla-central/search?q=.control+%3D&path=
[1]: https://searchfox.org/mozilla-central/search?q=setAttribute%28%22control%22&path=
Depends on: 1494529
I filed bug 1494529 to sort out text-label binding. Tweaking bug summary to reflect better what left here.
Summary: Merge text-base with text-label binding → Migrate text-link/label bindings to custom elements
Alright, I can do a preliminary check to see if registering a CE for label-control and text-label is likely to have an impact.
(In reply to Brian Grinstead [:bgrins] from comment #34)
> Alright, I can do a preliminary check to see if registering a CE for
> label-control and text-label is likely to have an impact.

yeah, thank you, I updated the bug summary to reflect approach taken in your last patch :) I don't know for sure if it could be converted into CE, because of performance concerns. Having said that, it seems there's no so many label@controls in UI [1], so possibly it's ok, however labels are also part of form controls like XUL checkboxes and radios.

https://searchfox.org/mozilla-central/search?q=%3Clabel.*control%3D&case=false&regexp=true&path=
Summary: Migrate text-link/label bindings to custom elements → Migrate text-link/label-control bindings to custom elements
Depends on: 1497601
Depends on: 1499476
There are some perf issues related to getting a JS reference to the binding parent (https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/content/widgets/text.xml#29), which triggers XBL construction on the parent.

This didn't used to be an issue because this function was only called once the label itself was constructed (which would be the same time as the parent).

I think we can work around this by making sure that everywhere that uses a <xul:label> in anon content uses [inherits] for accessKey, so that we can be sure that [accessKey] is always correct (and we don't need to read it from the bindingParent). This would also mean we don't need to set `control.labelElement = this;` and then mirror the accessKey value from the control onto the label at https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/content/widgets/general.xml#46-50 - we could check for a non-anon control by looking for [control=this.id] in the DOM to make sure we copy the accessKey across in that case.

The fact that the last anon <label> constructed inside a binding wins the race to be `control.labelElement` is a bit odd anyway (if anon content has more than one xul:label) - it probably makes sense to be more explicit about which label is intended to have the accessKey copied.
(In reply to Brian Grinstead [:bgrins] from comment #36)
> There are some perf issues related to getting a JS reference to the binding
> parent
> (https://searchfox.org/mozilla-central/rev/
> fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/content/widgets/text.
> xml#29), which triggers XBL construction on the parent.

The problem will be also solved if all "host" bindings are converted to CE, so we might want to wait on this bug until those are converted.

> This didn't used to be an issue because this function was only called once
> the label itself was constructed (which would be the same time as the
> parent).
> 
> I think we can work around this by making sure that everywhere that uses a
> <xul:label> in anon content uses [inherits] for accessKey, so that we can be
> sure that [accessKey] is always correct

yeah, this should also work
Attachment #9008478 - Attachment description: Bug 1448213 - WIP - Migrate description and label elements to Custom Elements → Bug 1448213 - WIP - Migrate label-control and text-link bindings to a Custom Element;
(In reply to alexander :surkov (:asurkov) from comment #37)
> (In reply to Brian Grinstead [:bgrins] from comment #36)
> > There are some perf issues related to getting a JS reference to the binding
> > parent
> > (https://searchfox.org/mozilla-central/rev/
> > fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/content/widgets/text.
> > xml#29), which triggers XBL construction on the parent.
> 
> The problem will be also solved if all "host" bindings are converted to CE,
> so we might want to wait on this bug until those are converted.

Yeah, but when converting host bindings I am sometimes seeing regressions that I think are at least partially related to eagerly constructing XBL from JS access on the <label> (which triggers accesskey formatting) :/. I think if we're able to get this landed it will make doing the host bindings earlier.

> > This didn't used to be an issue because this function was only called once
> > the label itself was constructed (which would be the same time as the
> > parent).
> > 
> > I think we can work around this by making sure that everywhere that uses a
> > <xul:label> in anon content uses [inherits] for accessKey, so that we can be
> > sure that [accessKey] is always correct
> 
> yeah, this should also work

I'm going to try this. I pushed up a version to phab.
Attachment #9008478 - Attachment description: Bug 1448213 - WIP - Migrate label-control and text-link bindings to a Custom Element; → Bug 1448213 - Migrate label-control and text-link bindings to a Custom Element;paolo
Depends on: 1508446
See Also: → replace-text-link
Depends on: 1465457

filing bug 1527495 for text-link binding

Summary: Migrate text-link/label-control bindings to custom elements → Migrate label-control bindings to custom elements
Depends on: 1527495
Summary: Migrate label-control bindings to custom elements → Migrate label-control binding to a Custom Element
Attachment #9008478 - Attachment description: Bug 1448213 - Migrate label-control and text-link bindings to a Custom Element;paolo → Bug 1448213 - Migrate label-control to a Custom Element
Depends on: 1534480
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b85a9bd9e728
Migrate label-control to a Custom Element r=surkov

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: