Closed Bug 1488620 Opened 6 years ago Closed 6 years ago

get rid of nsIDOMXULLabelElement

Categories

(Core :: XUL, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Whiteboard: [overhead:noted])

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
      No description provided.
Attachment #9006424 - Flags: review?(bugs)
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
(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).
Whiteboard: [overhead:noted]
(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-
(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.
ni? Olli for comment 7
Flags: needinfo?(bugs)
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.
(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
Attached 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+
https://hg.mozilla.org/mozilla-central/rev/ab213805a838
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: needinfo?(enndeakin)
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.