1590884 - Remove nsXULLabelFrame (XUL label without value attribute)

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Tim Nguyen :ntim]

nsXULLabelFrame is used for XUL <label>s without the value attribute (nsTextBoxFrame is used for <label>s with the value attribute). See: https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/layout/base/nsCSSFrameConstructor.cpp#4217-4227

I believe the only reason why nsXULLabelFrame exists is to register/unregister accesskeys inside the event state manager. There doesn't seem to be any special rendering attached to it. See: https://searchfox.org/mozilla-central/source/layout/xul/nsXULLabelFrame.cpp

I think it would make sense to move the accesskey registration out of the layout frame, preferably to a generic place where we can use it for HTML <label>s so we can start migrating XUL <label>s that don't use the value attribute.

Comment 1

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #0)

I think it would make sense to move the accesskey registration out of the layout frame, preferably to a generic place where we can use it for HTML <label>s so we can start migrating XUL <label>s that don't use the value attribute.

Actually, HTML <label> elements support the accesskey attribute, and it behaves exactly the same way as the XUL accesskey ones (the distinction that's done internally is Chrome vs. Content rather than HTML vs. XUL), so we might be able to simply get away with switching to HTML labels without worrying about moving the existing code elsewhere.

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1590884 - Remove nsXULLabelFrame platform code.

Comment 3

[Tim Nguyen :ntim]

bgrins pointed out that XUL labels without the value attribute also underline their accesskey, so the conversion would probably need to be done to a customized HTML label element or an HTML moz-label CE.

Comment 4

[Brian Grinstead [:bgrins]]

Wouldn't our existing XUL label CE continue to handle the underlining with this patch applied? Or is the problem that it will break accesskey handling without further changes (and switching to HTML would restore that).

Anyway, expanding the current XUL label CE to work for HTML labels - maybe with <html:label is="moz-label"> or something - seems pretty doable if that gets us around a problem. We could have that label even use display: moz-box and support flex attribute if that will make migration easier.

Comment 5

[Tim Nguyen :ntim]

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

Wouldn't our existing XUL label CE continue to handle the underlining with this patch applied?

Yes, it would.

Or is the problem that it will break accesskey handling without further changes (and switching to HTML would restore that).

Yep, that's exactly the problem. nsXULLabelFrame does calls to RegisterAccesskey/UnregisterAccesskey if the label has a control attribute and a accesskey attribute. Switching to HTML would fix the problem, since the accesskey attribute is supported on all HTML elements.

Anyway, expanding the current XUL label CE to work for HTML labels - maybe with <html:label is="moz-label"> or something - seems pretty doable if that gets us around a problem. We could have that label even use display: moz-box and support flex attribute if that will make migration easier.

That sounds doable, but the hard part is figuring out what usages are <label value> and what usages are <label>XXX</label>, especially in places where we already switched to fluent, a simple search/find replace gets much harder in those cases.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #5)

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

Wouldn't our existing XUL label CE continue to handle the underlining with this patch applied?

Yes, it would.

Or is the problem that it will break accesskey handling without further changes (and switching to HTML would restore that).

Yep, that's exactly the problem. nsXULLabelFrame does calls to RegisterAccesskey/UnregisterAccesskey if the label has a control attribute and a accesskey attribute. Switching to HTML would fix the problem, since the accesskey attribute is supported on all HTML elements.

Anyway, expanding the current XUL label CE to work for HTML labels - maybe with <html:label is="moz-label"> or something - seems pretty doable if that gets us around a problem. We could have that label even use display: moz-box and support flex attribute if that will make migration easier.

That sounds doable, but the hard part is figuring out what usages are <label value> and what usages are <label>XXX</label>, especially in places where we already switched to fluent, a simple search/find replace gets much harder in those cases.

How about a try push where we throw in the Custom Element when there's a non-empty textContent in the connectedCallback or when the textContent property is set: https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/toolkit/content/widgets/text.js#54. That'd probably get us pretty far.

Comment 7

[Brian Grinstead [:bgrins]]

Along with updating the toolkit custom elements which will generate a bunch of the labels at runtime: https://searchfox.org/mozilla-central/search?q=%3Clabel&case=false&regexp=false&path=toolkit%2Fcontent%2Fwidgets

Comment 8

[Magnus Melin [:mkmelin]]

In a larger scope, it would be nice if there was a way to actually underline html accesskeys automatically. (Lack of discoverability makes them almost useless as is.)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Magnus Melin [:mkmelin] from comment #8)

In a larger scope, it would be nice if there was a way to actually underline html accesskeys automatically. (Lack of discoverability makes them almost useless as is.)

I agree. There was some related discussion at https://groups.google.com/d/msg/mozilla.dev.platform/sQSF0Ms_X7g/gRubKwTCAQAJ. Anne, do you know (or could you point us to someone who might) if there has been discussions about standardizing visual accesskey rendering for html elements? We do this for XUL labels and not rendering the accesskey is a limitation that makes it harder for us to move to HTML in the browser chrome. We could do something chrome-only but I wonder if making something exposed to the web would be better.

This page explains the state of things, including options for rendering them: https://webaim.org/techniques/keyboard/accesskey#usersknow. XUL labels do a combination of "Underlining the letter within a word that activates the accesskey" and "Putting the accesskey in parentheses" (if the character doesn't appear in the word). We also only do this on windows/linux unless if a pref is set.

Comment 10

[Anne (:annevk)]

I think this is the first time I've seen this raised and searching across various repositories I didn't find anything, so I filed https://github.com/whatwg/html/issues/5042. I'd recommend going with a chrome-only extension to CSS for now and then iterating in that issue to hopefully find something that's mutually agreeable.

Comment 11

[Tim Nguyen :ntim]

Attachment: Bug 1590884 - Move XUL accesskey handling to DOM and remove nsXULLabelFrame.

Comment 12

[Tim Nguyen :ntim]

FWIW, I've only removed nsXULLabelFrame (so <label> without value attribute), I don't expect any rewriting or test changes needed from this.

<label value> uses nsTextBoxFrame which isn't removed by this patch.

Here's a ./mach try auto --no-artifact: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f08d1bc8ed0a04870357f297b429315c3933766b

Comment 13

[Tim Nguyen :ntim]

Hi Gijs and Brian, I was wondering if we used description[value] or description[control] anywhere. Do you know if there's an easy way to check that across the tree? (especially with these attributes set via fluent)

This patch preserves the old behaviour, so no issue here. Although if I don't need to worry about the <description> tag being used with [value] or [control], I can remove both some layout code (for giving description[value] a special layout frame) and some DOM accesskey logic (for description[value][control]) after this patch.

Comment 14

[Brian Grinstead [:bgrins]]

What I've done in the past if it's not greppable is make a patch that throws when that situation occurs and push to try with all mochitests. For the case of description we don't have a Custom Element so this would be in C++, and also watching for attribute change to catch dyamic changes like Fluent.

Comment 15

[Brian Grinstead [:bgrins]]

Just searching around for obvious consumers I see some test hits for [value] (https://searchfox.org/mozilla-central/search?q=description+value%3D&path=) and some actual hits for [control] (https://searchfox.org/mozilla-central/search?q=description+control%3D&path=). But I'd definitely double check for more with something like Comment 14.

Comment 16

[Tim Nguyen :ntim]

Thanks Brian! I've moved this to bug 1694564. After thinking a bit <description control> seems ok, seems like value or accesskey are the attributes to look out for. I've put up a C++ diagnostic patch there.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9094e2d390d
Move XUL accesskey handling to DOM and remove nsXULLabelFrame. r=emilio

Comment 19

[Tim Nguyen :ntim]

Attachment: Bug 1590884 - Extra small cleanups. DONTBUILD

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2ffe16dc047
Extra small cleanups. DONTBUILD

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f9094e2d390d
https://hg.mozilla.org/mozilla-central/rev/f2ffe16dc047

Comment 22

[Tim Nguyen :ntim]

Magnus, we now allow label:not([value]) and description:not([value]) to get their CSS display value overridden by other stylesheets. So if some CSS on your side did that before, it will now actually have an effect (unlike before where only display: none worked).

This caused bug 1694936 on our side. You may want to check if there was some display override that suddenly turned active.

The default behaviour of label:not([value]) and description:not([value]) remains unchanged however.

Comment 23

[Magnus Melin [:mkmelin]]

Thanks, will keep an eye out for it! I'm sure there must be some cases of it out there, but hard to fine them.