get rid of nsIDOMXULLabelElement

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [overhead:noted])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 months ago
Posted patch patch (obsolete) — Splinter Review
Attachment #9006424 - Flags: review?(bugs)
(Assignee)

Updated

7 months ago
Comment on attachment 9006424 [details] [diff] [review]
patch

Review of attachment 9006424 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/text.xml
@@ +76,4 @@
>              }
>              if (control) {
>                control.labelElement = this;
> +              var controlAccessKey = control.getAttribute("accesskey");

I believe this could lead to an out of date access key in the the case where a control gets set on a label, and then the control's accessKey changes later on. I doubt this happens in the chrome, but I'm not sure. As an alternative I could imagine updating nsTextBoxFrame::UpdateAccesskey to check for the [control] attr and if it exists then getElementById for the control and read accessKey from that node (just as the current XBL accessKey does). I don't feel too strongly either way - what do you think?

@@ +235,5 @@
>        <field name="mInsertSeparator"/>
>        <field name="mAlwaysAppendAccessKey">false</field>
>  
> +      <property name="accessKey"
> +                onget="">

Was this [onget] unintentionally added? If we keep the <getter> then we generally wouldn't want the attr as well.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 months ago
(In reply to Brian Grinstead [:bgrins] from comment #1)

> ::: toolkit/content/widgets/text.xml
> @@ +76,4 @@
> >              }
> >              if (control) {
> >                control.labelElement = this;
> > +              var controlAccessKey = control.getAttribute("accesskey");
> 
> I believe this could lead to an out of date access key in the the case where
> a control gets set on a label,

formatAccessKey() is called, see [1].

> and then the control's accessKey changes
> later on.

in this case formatAccessKey() will called as well, see [2] and [3]

> I doubt this happens in the chrome, but I'm not sure. As an
> alternative I could imagine updating nsTextBoxFrame::UpdateAccesskey to
> check for the [control] attr and if it exists then getElementById for the
> control and read accessKey from that node (just as the current XBL accessKey
> does). I don't feel too strongly either way - what do you think?

taking into account the links above the taken approach looks workable?

> @@ +235,5 @@
> >        <field name="mInsertSeparator"/>
> >        <field name="mAlwaysAppendAccessKey">false</field>
> >  
> > +      <property name="accessKey"
> > +                onget="">
> 
> Was this [onget] unintentionally added? If we keep the <getter> then we
> generally wouldn't want the attr as well.

it was an artifact, I was thinking to switch to onget() version, will remove it

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/text.xml?q=toolkit%2Fcontent%2Fwidgets%2Ftext.xml&redirect_type=direct#273
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/general.xml#49
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/text.xml#256
Priority: -- → P3
(In reply to alexander :surkov (:asurkov) from comment #2)
> taking into account the links above the taken approach looks workable?

The cases I'm thinking of are:

```
<button id="foo" accesskey="A" />
<label control="foo" />

// due to constructor calling formatAccessKey:
label.getAttribute("accesskey") == "A"

// AFAICT formatAccessKey won't get re-called here since the control hasn't changed - am I missing something?
button.setAttribute("accesskey", "B");
label.getAttribute("accesskey") == "A";

// Even if formatAccessKey did re-called, we wouldn't hit the `if (controlAccessKey)` condition so it wouldn't be removed in this case:
button.removeAttribute("accesskey");
label.getAttribute("accesskey") == "A" 
```

Again, I'm not sure if this ever happens in practice (accesskey changes seem like they'd be rare, and it may be the case when they do happen they always run through the XBL property setter instead of directly on an attr).

Updated

7 months ago
Whiteboard: [overhead:noted]
(Assignee)

Comment 4

7 months ago
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to alexander :surkov (:asurkov) from comment #2)
> > taking into account the links above the taken approach looks workable?
> 
> The cases I'm thinking of are:
> 
> ```
> <button id="foo" accesskey="A" />
> <label control="foo" />
> 
> // due to constructor calling formatAccessKey:
> label.getAttribute("accesskey") == "A"
> 
> // AFAICT formatAccessKey won't get re-called here since the control hasn't
> changed - am I missing something?
> button.setAttribute("accesskey", "B");
> label.getAttribute("accesskey") == "A";
> 
> // Even if formatAccessKey did re-called, we wouldn't hit the `if
> (controlAccessKey)` condition so it wouldn't be removed in this case:
> button.removeAttribute("accesskey");
> label.getAttribute("accesskey") == "A" 
> ```

Ah, now I see your point. I think this is "illegal" to change accesskey via DOM attribute on XUL controls, you have to use a property for that. It seems even now button.setAttribute("accesskey", "B") won't trigger XUL label accesskey update (not sure about behavioral part, but it looks like it doesn't update UI).

> Again, I'm not sure if this ever happens in practice (accesskey changes seem
> like they'd be rare, and it may be the case when they do happen they always
> run through the XBL property setter instead of directly on an attr).

Yeah, that's the case I think. Neil, is it correct?
Flags: needinfo?(enndeakin)
(In reply to alexander :surkov (:asurkov) from comment #4)
> Ah, now I see your point. I think this is "illegal" to change accesskey via
> DOM attribute on XUL controls, you have to use a property for that. It seems
> even now button.setAttribute("accesskey", "B") won't trigger XUL label
> accesskey update (not sure about behavioral part, but it looks like it
> doesn't update UI).

OK, if that already isn't working then I'm happy to keep the behavior. We may want to revisit how some of this gets wired together if/when we port this to C++ as a XUL element subclass but seems fine as-is.
Comment on attachment 9006424 [details] [diff] [review]
patch

>@@ -200,17 +199,9 @@ bool
> nsTextBoxFrame::UpdateAccesskey(WeakFrame& aWeakThis)
> {
>     nsAutoString accesskey;
>-    nsCOMPtr<nsIDOMXULLabelElement> labelElement = do_QueryInterface(mContent);
>-    NS_ENSURE_TRUE(aWeakThis.IsAlive(), false);
>-    if (labelElement) {
>-        // Accesskey may be stored on control.
>-        labelElement->GetAccessKey(accesskey);
>-        NS_ENSURE_TRUE(aWeakThis.IsAlive(), false);
>-    } else {
>-        mContent->AsElement()->GetAttr(kNameSpaceID_None,
>-                                       nsGkAtoms::accesskey,
>-                                       accesskey);
>-    }
>+    mContent->AsElement()->GetAttr(kNameSpaceID_None,
>+                                   nsGkAtoms::accesskey,
>+                                   accesskey);

So we clearly do change behavior quite a bit. Why?
We deal with accesskey changes in many places, but is that all now not needed or what?


>@@ -231,17 +235,11 @@
>       <field name="mInsertSeparator"/>
>       <field name="mAlwaysAppendAccessKey">false</field>
> 
>-      <property name="accessKey">
>+      <property name="accessKey"
>+                onget="">
what is this onget
Attachment #9006424 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

7 months ago
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 9006424 [details] [diff] [review]
> patch
> 
> >@@ -200,17 +199,9 @@ bool
> > nsTextBoxFrame::UpdateAccesskey(WeakFrame& aWeakThis)
> > {
> >     nsAutoString accesskey;
> >-    nsCOMPtr<nsIDOMXULLabelElement> labelElement = do_QueryInterface(mContent);
> >-    NS_ENSURE_TRUE(aWeakThis.IsAlive(), false);
> >-    if (labelElement) {
> >-        // Accesskey may be stored on control.
> >-        labelElement->GetAccessKey(accesskey);
> >-        NS_ENSURE_TRUE(aWeakThis.IsAlive(), false);
> >-    } else {
> >-        mContent->AsElement()->GetAttr(kNameSpaceID_None,
> >-                                       nsGkAtoms::accesskey,
> >-                                       accesskey);
> >-    }
> >+    mContent->AsElement()->GetAttr(kNameSpaceID_None,
> >+                                   nsGkAtoms::accesskey,
> >+                                   accesskey);
> 
> So we clearly do change behavior quite a bit. Why?
> We deal with accesskey changes in many places, but is that all now not
> needed or what?

Do we change the behavior here? I added changes into XUL binding, so it keeps accesskey attribute on a XUL label always updated (previosly it was used to be updated only when accesskey setter property was triggered either on label or its control).

> >@@ -231,17 +235,11 @@
> >       <field name="mInsertSeparator"/>
> >       <field name="mAlwaysAppendAccessKey">false</field>
> > 
> >-      <property name="accessKey">
> >+      <property name="accessKey"
> >+                onget="">
> what is this onget

yeah, Brian caught it in comment 2 and I promised to remove in comment 3 and I did it locally already :) Let me know if I need to upload a new patch with the change.
(Assignee)

Comment 8

7 months ago
ni? Olli for comment 7
Flags: needinfo?(bugs)
Blocks: 1446831
FYI, you may want to do a talos push with your updated local version - I'm seeing a ts_paint_webext regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=01fe8e05702d3fa128d0ab7ca125c4aa22bbd50e&newProject=try&newRevision=bae34e0ea1cb144d8b5ed0fa999c4c34216503dc&framework=1&showOnlyImportant=1. Doesn't make a lot of sense to me that this would regress, worth another push.
(Assignee)

Comment 10

6 months ago
(In reply to Brian Grinstead [:bgrins] from comment #9)
> FYI, you may want to do a talos push with your updated local version - I'm
> seeing a ts_paint_webext regression:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=01fe8e05702d3fa128d0ab7ca125c4aa
> 22bbd50e&newProject=try&newRevision=bae34e0ea1cb144d8b5ed0fa999c4c34216503dc&
> framework=1&showOnlyImportant=1. Doesn't make a lot of sense to me that this
> would regress, worth another push.

yeah, the patch shouldn't have any effect on perf, I started a fresh talos run just in case, https://treeherder.mozilla.org/#/jobs?repo=try&revision=04dbe44ed2f7d3a935193a4fee9a3ff4fcad5473
(Assignee)

Comment 11

6 months ago
Posted patch patchSplinter Review
a fresh patch with onget thing is addressed
Attachment #9006424 - Attachment is obsolete: true
Attachment #9009018 - Flags: review?(bugs)
Comment on attachment 9009018 [details] [diff] [review]
patch

All this looks regression prone, but at least most of the regressions should be in FF UI.
And the nsTextBoxFrame::UpdateAccesskey change sure isn't covered by the XBL change. nsTextBoxFrame::UpdateAccesskey after all triggers a new reflow and what not if needed.
Flags: needinfo?(bugs)
Attachment #9009018 - Flags: review?(bugs) → review+

Comment 14

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab213805a838
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.