Closed Bug 1532651 Opened 6 years ago Closed 4 years ago

4.43% about_preferences_basic (osx-10-10) regression on push 623c6ca6ed6cf8128cebf9944524f3be13e0d4d2 (Fri Mar 1 2019)

Categories

(Core :: XUL, defect, P2)

Product:
Core
Component:
XUL
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX
Project Flags:
Performance Impact medium
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fix-optional

People

(Reporter: Bebe, Unassigned)

References

(Regression)

Details

(5 keywords)

Attachments

(2 files)

Bug 1532651 - replace xul:checkbox on html:input@type=checkbox in browser preferences
47 bytes, text/x-phabricator-request
Details | Review
time-during-privacy-pane-load.png
117.72 KB, image/png
Details

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=623c6ca6ed6cf8128cebf9944524f3be13e0d4d2

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

4% about_preferences_basic osx-10-10 opt e10s stylo 164.23 -> 171.50

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=19728

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Performance_sheriffing/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Performance_sheriffing/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Performance_sheriffing/Talos/RegressionBugsHandling

Blocks: 1455433, 1521698
Component: General → XUL
Product: Testing → Core
Version: Version 3 → unspecified

Alex, can you take a look?

Flags: needinfo?(surkov.alexander)
Priority: -- → P2

(In reply to Neil Deakin from comment #1)

Alex, can you take a look?

yeah, was looking into it. Basically two things come to my mind:

  1. move listeners into connectedCallback, replace DOMParser on document.createElement stuff, it might be a bit faster. If it wont' work, then
  2. maybe we could try to replace xul:checkbox on html:input@type="checkbox" in preferences panel.
Flags: needinfo?(surkov.alexander)

Matt, does this approach looks (XUL:checkbox to HTML:input conversion) reasonable and worth to continue (see the patch attached)?

Flags: needinfo?(MattN+bmo)

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

(In reply to Neil Deakin from comment #1)

Alex, can you take a look?

yeah, was looking into it. Basically two things come to my mind:

  1. move listeners into connectedCallback, replace DOMParser on document.createElement stuff, it might be a bit faster. If it wont' work, then
  2. maybe we could try to replace xul:checkbox on html:input@type="checkbox" in preferences panel.

Were these ideas based on profiles from Talos? I think it would be good to compare them.

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

Matt, does this approach looks (XUL:checkbox to HTML:input conversion) reasonable and worth to continue (see the patch attached)?

It seems like a fair amount of work and we're running close to the code freeze… I don't know much about <checkbox> or prefs tbh. so I'm probably not the best person to ask. :jaws can speak to about:preferences… not sure who else could provide input while bgrins is gone.

Flags: needinfo?(MattN+bmo)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)

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

(In reply to Neil Deakin from comment #1)

Alex, can you take a look?

yeah, was looking into it. Basically two things come to my mind:

  1. move listeners into connectedCallback, replace DOMParser on document.createElement stuff, it might be a bit faster. If it wont' work, then
  2. maybe we could try to replace xul:checkbox on html:input@type="checkbox" in preferences panel.

Were these ideas based on profiles from Talos? I think it would be good to compare them.

no, just ideas, potentially bottleneck places.

I did couple a talos tries for them, neither of them shows any improvement:

  1. no XULParse thing:
    changeset: https://hg.mozilla.org/try/rev/45db02116b809713593f02e05b9b83076eaad1b4
    perf: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f752d31afe7ba09dc0abc2363ab0b9a1dbf9018f&newProject=try&newRevision=5adcaccab4f1273fb84d266a58c50e33649064bf&framework=1&showOnlyImportant=1

  2. no XULPrase thing + empty constructor
    changeset: https://hg.mozilla.org/try/rev/7442617f2d8a17a35d0861140e18afd16087d569
    perf: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f752d31afe7ba09dc0abc2363ab0b9a1dbf9018f&newProject=try&newRevision=cac241ccf4b3a81b2ccc6d9d622833ae557e6d87&framework=1&showOnlyImportant=1

Also I ran the test locally, and it seems time related with MozCheckbox is spent during MozCheckbox construction, i.e. MozCheckbox -> Mixin -> BaseControl calls. That makes me thing that just having a custom element for a checkbox on preferences page can be an issue by itself.

I'll run a talos for HTML:input just in case to see if there's any sign of improvement.

It might be worth looking at removing the inheritedAttributes stuff and see if there's some perf improvements from that.

The attributes are mostly only inherited for easier styling, so it can be avoided.

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

It might be worth looking at removing the inheritedAttributes stuff and see if there's some perf improvements from that.

The attributes are mostly only inherited for easier styling, so it can be avoided.

it seems you're right, inheritedAttributes hits a lot. Here's some talos runs:

  1. no ctor no connectedCallback: 10% win, https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=818d464fd72896d00a76a839f445f1a6d3a3ce39&newProject=try&newRevision=0b0617cf41f330be55f3fa3ce39601c438c7e1f5&framework=1&showOnlyImportant=1
  2. no connnectedCallback: ~6% win, https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=818d464fd72896d00a76a839f445f1a6d3a3ce39&newProject=try&newRevision=987b6f112ce7131864e8316f07ca544f10a7294e&framework=1&showOnlyImportant=1
  3. no attribute inheritance, 4.2 win, what puts us about into same place we were before regression, https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=818d464fd72896d00a76a839f445f1a6d3a3ce39&newProject=try&newRevision=ee62e6bca107d83b925484fa1e236462a873e166&framework=1&showOnlyImportant=1

I started a new talos with plain inherit attrs implementations, let's see how it goes https://treeherder.mozilla.org/#/jobs?repo=try&revision=523feb8bae71612e00dba0a012ce050b7781383c

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

I started a new talos with plain inherit attrs implementations, let's see how it goes https://treeherder.mozilla.org/#/jobs?repo=try&revision=523feb8bae71612e00dba0a012ce050b7781383c

This getter name should change to observedAttributes: https://hg.mozilla.org/try/rev/3de9a5fe62dcc5411d9869c321b336b5990732b4#l1.11 (as it is it's not calling attributeChangedCallback).

Also, if initializeAttributeInheritance does turn out to be slower than the vanilla calls to attributeChangedCallback (with the fix from Comment 9 applied), then we should also see if implementing https://bugzilla.mozilla.org/show_bug.cgi?id=1528268 fixes this instead of of needing to opt out of it for certain cases.

Still don't understand why this is OSX only (which is the only platform where we aren't doing accesskey formatting by default, which seems like the bulk of the work from this attribute inheritance).

If this is related to attribute inheritance then I wonder if https://github.com/projectfluent/fluent.js/issues/300 will help here.

By the way, you can update the fuzzy query to do talos-chrome-e10s and it will only run the 'c' suite (where about_preferences_basic runs). That will save some resources and also cut down on noise in the compare UI.

This is from https://perfht.ml/2HiNJoE. A couple notes:

  1. This is when loading about:preferences#privacy which is apparently what is causing the regression (Alex can give more details)
  2. We have 3ms of self time in initializeAttributeInheritance which I'm guessing are the querySelector calls. I wonder if we could somehow optimize the selectors.
  3. Odd that string.split shows up at all here. We could store an array instead of the split string in initializeAttributeInheritance to make it so the split only happens once instead of on each inheritance

I wonder if it would help to create the child DOM nodes in C++ when the element is bound to the tree instead of inside JS/connectedCallback. I don't love having to implement attribute inheritance in C++ as well (since we'd then have 2 separate implementations for that), but maybe with the optimizations in Bug 1528268 it would still be fine in JS.

Though I'm still confused why this is OSX only (if this was an over-eager XBL construction issue with labels it would have hit Windows and Linux harder since they do actual access key formatting). I wonder if we are hitting a cliff that would be fixed by ASAP mode (as in Bug 1504139).

Alex was also looking at migrating the <checkbox> instances in the privacy subpage to use html:input just to get some data about if that would help.

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

Though I'm still confused why this is OSX only (if this was an over-eager XBL construction issue with labels it would have hit Windows and Linux harder since they do actual access key formatting).

Now that you mentioned that: this is not OSX-specific. It also happens on Windows 10 QR and Linux 64bit QR.

Regressions:

4% about_preferences_basic osx-10-10 opt e10s stylo 164.23 -> 171.50
4% about_preferences_basic linux64-pgo-qr opt e10s stylo 127.85 -> 132.87
3% about_preferences_basic windows10-64-pgo-qr opt e10s stylo 106.66 -> 109.52

OS: Unspecified → All

replacing XUL:checkboxes on HTML:input in #privacy preferences shows some good generic wins [1]:
os-x: -7.5%,
linux: -3,3%,
win: -4.7

I do see some low confidence wins in general (for all tests), but #privacy subtest shows -25%, whose tendency should be trustworthy.

:jaws, how do you feel about an idea to conver checkboxes to HTML?

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=4f1fe36d5dd85489cd565f50e96bf207b89c2ce4&newProject=try&newRevision=f5d6041ff2c6725c3df741a77f572b6d9a0b4447&originalSignature=1659171&newSignature=1659171&framework=1

Flags: needinfo?(jaws)

Brian also suggested to try to replace XUL checkbox CE content by HTML, if it gives any good results it's worth to check.

My only concerns with html-ifying the checkboxes are (a) we couldn't get on 67 (could be fine if jaws is OK with that) and (b) it may break the highlightable search text feature (I haven't tested that but there are two copies of the text specifically for highlighting results in search IIRC).

The other idea I had was to drop the <image> from the checkbox markup and inherited attributes since it seems unused mostly. This might give us enough wins to get back to the status quo and would be very lightweight / upliftable.

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

replacing XUL:checkboxes on HTML:input in #privacy preferences shows some good generic wins [1]:
os-x: -7.5%,
linux: -3,3%,
win: -4.7

I do see some low confidence wins in general (for all tests), but #privacy subtest shows -25%, whose tendency should be trustworthy.

:jaws, how do you feel about an idea to conver checkboxes to HTML?

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=4f1fe36d5dd85489cd565f50e96bf207b89c2ce4&newProject=try&newRevision=f5d6041ff2c6725c3df741a77f572b6d9a0b4447&originalSignature=1659171&newSignature=1659171&framework=1

IMHO, converting in-content prefs checkboxes to HTML shouldn't pose a lot of risk given that the JS code to handle HTML inputs has been there for a while now: https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#447 and https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#268 . The CSS has been there for a while too.

But because of Fluent, all the checkbox strings would need to change string IDs (see my comment on the phabricator revision), which makes it difficult to uplift.

Other than that, this would be my preferred solution.

Brian also suggested to try to replace XUL checkbox CE content by HTML, if it gives any good results it's worth to check.

HTML-ifying the CE content seems more risky since this affects all the checkboxes, and not just the preference ones, but could avoid string ID changes. There's also not that much precedent of using HTML content in XUL CEs.

The other idea I had was to drop the <image> from the checkbox markup and inherited attributes since it seems unused mostly. This might give us enough wins to get back to the status quo and would be very lightweight / upliftable.

Dropping the .checkbox-icon from the CE (and removing the surrounding .checkbox-label-box) could work, but this is actually used in about:preferences so those usages will need to be re-implemented (.checkbox-label::before could be used for that).

But because of Fluent, all the checkbox strings would need to change string IDs (see my comment on the phabricator revision), which makes it difficult to uplift.

I thought that we have migration capabilities for fluent->fluent migrations. NI on flod?

Flags: needinfo?(francesco.lodolo)

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

IMHO, converting in-content prefs checkboxes to HTML shouldn't pose a lot of risk given that the JS code to handle HTML inputs has been there for a while now: https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#447 and https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#268 . The CSS has been there for a while too.

I didn't realize that we were already handling HTML checkbox inputs - that would lower the risk quite a bit here.

Dropping the .checkbox-icon from the CE (and removing the surrounding .checkbox-label-box) could work, but this is actually used in about:preferences so those usages will need to be re-implemented (.checkbox-label::before could be used for that).

I don't think it'd be too hard to update, at least based on the consumers I've found from a brief search. I gave it a quick try at https://hg.mozilla.org/try/rev/3c1b841ac76f15859aac143d8caaf97dc8ae8ab4. Will do a talos push to see if that is worth looking into further.

(In reply to (Unavailable until March 21) Brian Grinstead [:bgrins] from comment #25)

Dropping the .checkbox-icon from the CE (and removing the surrounding .checkbox-label-box) could work, but this is actually used in about:preferences so those usages will need to be re-implemented (.checkbox-label::before could be used for that).

I don't think it'd be too hard to update, at least based on the consumers I've found from a brief search. I gave it a quick try at https://hg.mozilla.org/try/rev/3c1b841ac76f15859aac143d8caaf97dc8ae8ab4. Will do a talos push to see if that is worth looking into further.

It doesn't seem to give any strong win overall on osx: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f0d16d9ba0c5f571b0d7aa12ef27a8466e377d8b&newProject=try&newRevision=61ac9707c578d04b3b95cffb3adc102f29c10717&framework=1. Though looking at subtests I do see a strong win on #home so it's probably worth pursuing if we do keep using <checkbox> CE: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f0d16d9ba0c5f571b0d7aa12ef27a8466e377d8b&newProject=try&newRevision=61ac9707c578d04b3b95cffb3adc102f29c10717&originalSignature=1659171&newSignature=1659171&framework=1

Dropping the checkbox-icon and checkbox-label-box, and using normal DOM construction instead of parseXULToFragment shows a strong 10% win on Windows 10: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f0d16d9ba0c5f571b0d7aa12ef27a8466e377d8b&newProject=try&newRevision=835996635d60660c6ea742e08ee4a6918c76c774&framework=1&showOnlyImportant=1. Again it doesn't seem to show up on OSX but something for sure worth pursuing if we want to keep the basic XUL checkbox impl. WIP rev at: https://hg.mozilla.org/try/rev/500ba81d1dbe790cab05731a0a45d13689e41b18.

OTOH I'm not against migrating to html input type=check at all. There are some details to figure out (like if we want to wrap it up into a CE or just inline the label/input everywhere). I'm also assuming the migration will be a lot of work (if not within prefs, then outside of prefs UI) and not upliftable to 67. But if someone wanted to take that on it'd be a good option for sure.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)

But because of Fluent, all the checkbox strings would need to change string IDs (see my comment on the phabricator revision), which makes it difficult to uplift.

I thought that we have migration capabilities for fluent->fluent migrations. NI on flod?

We don't, and given the current workload and number of concurrent projects, I'm not sure when we'll have (bug 1453557).

Do we have a number of how many strings are affected by such a change? If it's large, I might ask to consider that bug as a blocker.

Flags: needinfo?(francesco.lodolo)

(In reply to (Unavailable until March 21) Brian Grinstead [:bgrins] from comment #27)

Dropping the checkbox-icon and checkbox-label-box, and using normal DOM construction instead of parseXULToFragment shows a strong 10% win on Windows 10: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f0d16d9ba0c5f571b0d7aa12ef27a8466e377d8b&newProject=try&newRevision=835996635d60660c6ea742e08ee4a6918c76c774&framework=1&showOnlyImportant=1. Again it doesn't seem to show up on OSX but something for sure worth pursuing if we want to keep the basic XUL checkbox impl. WIP rev at: https://hg.mozilla.org/try/rev/500ba81d1dbe790cab05731a0a45d13689e41b18.

It appears these two things make together quite a good improvement. Let's have a new bug for it?

I am generally okay with using html:input@type=checkbox, but I'm doubtful we would get the same accesskey support with html:input. The attached patch (https://hg.mozilla.org/try/rev/8d70212dd8ff6e1474db4ee64c929ee6d6e90915).

But the current accesskey support for checkboxes doesn't work well for me in local testing anyways.

If I try Shift+Alt+c to focus/toggle

<checkbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="deleteOnClose" data-l10n-id="sitedata-delete-on-close" preference="network.cookie.lifetimePolicy" onsyncfrompreference="return gPrivacyPane.readDeleteOnClose();" onsynctopreference="return gPrivacyPane.writeDeleteOnClose();" flex="1" label="Delete cookies and site data when Nightly is closed" accesskey="c"><image class="checkbox-check"/><hbox class="checkbox-label-box" flex="1"><image class="checkbox-icon"/><label class="checkbox-label" flex="1" accesskey="c">Delete cookies and site data when Nightly is closed</label></hbox></checkbox>

, nothing happens. Clicking on the checkbox first, then pressing Shift+Alt+c cycles through various controls but never brings focus to this checkbox nor toggles it.

Flags: needinfo?(jaws) → needinfo?(surkov.alexander)

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #30)

I am generally okay with using html:input@type=checkbox, but I'm doubtful we would get the same accesskey support

I think using XUL label control instead of HTML label would solve the accesskey underline problem. Not sure about actual keyboard support though.

XUL:label doesn't seems working well with HTML:input. It requires some code adjustments.

  1. html:input doesn't expose accessible name, accessible relations and accesskeys to the assistive technologies. aria-labelledby can fix accessible name and accessible relations, but it doesn't address accesskeys issue. It requires some c++ changes in accessibility if we want to support XUL:labels for HTML controls.

  2. clicking xul:label doesn't change HTML checkbox value. It seems like an easy fix, text.xml@click handler should be adjusted to handle HTML:input

  3. accesskeys placed on XUL:label don't work. I suppose there's some c++ part that doesn't find related HTML:input. I guess, putting accesskeys both on HTML:input and XUL:label might work out. Keeping accesskey on HTML:input requires more adjustments in text.xml label binding. Note, the latter approach will address a11y accesskey issue as well.

Flags: needinfo?(surkov.alexander)

Some more accesskey issue findings.

So if accesskey is placed both on xul:label and a control, then accesskey doesn't really work, xul:label@accesskey is ignored , because the control goes next in accesskey targets (shouldActivate is false, label is not focusable in [1]).

If xul:label is the only element having an accesskey, then accesskey works, however the behavior is slightly different from the existing one, i.e. accesskey activation used to change checkbox value and also focus it, blue border around it. However if xul:label@accesskey is activated, then checkbox value is changed and checkbox is focused, but no blue border, because it's a mouse interaction I suppose. I guess it shall be considered as a bug.

[1] https://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#1011

Neil, curious if you're familiar with EventStateManager.cpp accesskey handling code. Is it expected that both label and control are registered for accesskey handling (LookForAccessKeyAndExecute)? If shouldActivate is ever set to false, because another accesskey target goes next, then it seems the method never reaches the next target to process accesskey.

Flags: needinfo?(enndeakin)

Don't know offhand, but masayuki might have some insight.

Flags: needinfo?(enndeakin) → needinfo?(masayuki)

Hmm, I don't remember around that well. I'll check it next week, but I can work only when my condition is not so bad. So, I'd be sorry if I took long time.

Whiteboard: [qf]
Flags: needinfo?(masayuki)

surkov: Ping on where this stands...
masayuki - you NI'd yourself 8 months ago, so I re-NI'd you.

Thanks!

Flags: needinfo?(surkov.alexander)
Flags: needinfo?(masayuki)
Whiteboard: [qf] → [qf:p2:responsiveness]

Sorry, I completely forgot this bug.

As far as I investigated, the inner loop is odd to me. For example, it's simply reproducible with HTML.

  1. <label accesskey="m"><input type="checkbox" accesskey="m">label</label>
  2. <input type="checkbox" accesskey="m" id="c"><label accesskey="m" for="c">label</label>
  3. <label accesskey="m" for="c">label</label><input type="checkbox" accesskey="m" id="c">
    With the case #1 and #3, the accesskey completely does not work. With the case #2, the <input> element just gets focus, not toggled. Chrome works fine with all cases. I think that this is bug 554104.
Flags: needinfo?(masayuki)

The inner loop was added by bug 143065.

If accesskey issue is not uneasy to solve with all variations of XUL:label/HTML:input@type=checkbox, then I suppose there's another option to try: extend HTML label by a custom element to underline an accesskey similar to XUL label, and then switch the whole thing to HTML:input@type="checkbox"/extended HTML:label.

Flags: needinfo?(surkov.alexander)

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

If accesskey issue is not uneasy to solve with all variations of XUL:label/HTML:input@type=checkbox, then I suppose there's another option to try: extend HTML label by a custom element to underline an accesskey similar to XUL label, and then switch the whole thing to HTML:input@type="checkbox"/extended HTML:label.

I filed Bug 1602095 to experiment with this, at least for the textContent (not [value]) variation of label. Right now the implementation lives in a web page but should be extendable into text.js (and possibly able to share some of the xul label functionality with a mixin). Is that something you'd be interested in picking up? I know Mark (cc'ed) wanted to be able to use something like this for the about:addons UI to avoid using the xul namespace there.

Flags: needinfo?(surkov.alexander)
See Also: → 1602095

(In reply to (Mostly unavailable through Jan 1) Brian Grinstead [:bgrins] from comment #41)

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

If accesskey issue is not uneasy to solve with all variations of XUL:label/HTML:input@type=checkbox, then I suppose there's another option to try: extend HTML label by a custom element to underline an accesskey similar to XUL label, and then switch the whole thing to HTML:input@type="checkbox"/extended HTML:label.

I filed Bug 1602095 to experiment with this, at least for the textContent (not [value]) variation of label. Right now the implementation lives in a web page but should be extendable into text.js (and possibly able to share some of the xul label functionality with a mixin). Is that something you'd be interested in picking up? I know Mark (cc'ed) wanted to be able to use something like this for the about:addons UI to avoid using the xul namespace there.

I can try to squeeze it somewhere between fission work. How urgent the feature is? Does it block the work?

Flags: needinfo?(surkov.alexander)

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

(In reply to (Mostly unavailable through Jan 1) Brian Grinstead [:bgrins] from comment #41)

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

If accesskey issue is not uneasy to solve with all variations of XUL:label/HTML:input@type=checkbox, then I suppose there's another option to try: extend HTML label by a custom element to underline an accesskey similar to XUL label, and then switch the whole thing to HTML:input@type="checkbox"/extended HTML:label.

I filed Bug 1602095 to experiment with this, at least for the textContent (not [value]) variation of label. Right now the implementation lives in a web page but should be extendable into text.js (and possibly able to share some of the xul label functionality with a mixin). Is that something you'd be interested in picking up? I know Mark (cc'ed) wanted to be able to use something like this for the about:addons UI to avoid using the xul namespace there.

I can try to squeeze it somewhere between fission work. How urgent the feature is? Does it block the work?

I don't think it's urgent, but it's on my radar as blocking part of the XULElement->HTMLElement effort (Bug 1563415), and Mark's for the new about:addons UI.

Mark, is there any particular bug involving the move away from xul:label in the addons UI? Also, is it important that the widget is content-priv in addition to HTML, or does a chrome-priv HTML element help? FWIW this functionality is supported in content, but the problem is pref lookups we do regarding underlining, so we'd need some way to load those settings in content environments (or change them into static values based on platform and remove the prefs).

Flags: needinfo?(mstriemer)

I don't have a bug for about:addons since it doesn't really cause any problems there since it's running in chrome-priv. It does block reusing the panel/menu component that we have there in content-priv HTML pages, like about:logins.

Not having the accesskey underlining in HTML is a change from what we used to do, so maybe there's some added priority to avoid regressions as things get converted. One other case I've noticed is <xul:button> where we would show the accesskey but <html:button> does not. This only occurs once (I believe) in about:addons but if there was a bigger effort on something like about:preferences it occurs there a lot.

Flags: needinfo?(mstriemer)

Same here. Not sure what flags should be set and this is 8 months old. No perfherder graph available anymore.

Flags: needinfo?(fstrugariu)

Tracking Flags: are set and updated. nothing to do here

Flags: needinfo?(fstrugariu)

:masayuki are you still actively working on this?

Flags: needinfo?(masayuki)

No, I'm too busy on beforeinput.

Flags: needinfo?(masayuki)

:bgrinstead is there someone else that can work on this? perhaps we can accept and close this regression as WONTFIX (as it has long since shipped to users), and open a new bug related to the potential performance improvements discussed here?

Flags: needinfo?(bgrinstead)

(In reply to Dave Hunt [:davehunt] [he/him] ⌚BST from comment #49)

:bgrinstead is there someone else that can work on this? perhaps we can accept and close this regression as WONTFIX (as it has long since shipped to users), and open a new bug related to the potential performance improvements discussed here?

Yeah, this has long shipped and some other optimizations have landed in the meantime (like Bug 1528268 and also generally being a lot more lazy with when DOM is created / initialized).

I don't think there's value in keeping this open anymore. As far as I can tell any accesskey work would be done in Bug 554104, and Bug 1602095 is opened to stop using (some of the) xul labels, so I don't think we need any new bugs filed.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(bgrinstead)
Resolution: --- → WONTFIX
See Also: → 1528268
Has Regression Range: --- → yes
Performance Impact: --- → P2
Keywords: perf:responsiveness
Whiteboard: [qf:p2:responsiveness]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: