1489748 - [de-xbl] Migrate mail-headerfield and mail-urlfield to custom element.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

binding link - https://searchfox.org/comm-central/source/mail/base/content/mailWidgets.xml#468

Comment 2

[Arshad Khan [:arshad]]

Attachment: mail-headerfield.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 4

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 5

[Arshad Khan [:arshad]]

mail-urlfield binding link - https://searchfox.org/comm-central/source/mail/base/content/mailWidgets.xml#501

Comment 6

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9007449 [details] [diff] [review]
mail-field.patch

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

I get a "ReferenceError: MozXULElement is not defined", and then all the header subjects are blank.

My tree is not super new, but I would guess that's not the problem.

::: mail/base/content/mailWidgets.js
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* global MozXULElement */

Not sure what you mean, but can we remove this comment?

@@ +38,5 @@
> +
> +  set headerValue(val) {
> +    let valueNode = this.querySelector(".headerValue");
> +
> +    // Since the control attribute points to the <mail-headerfield>

need to update this comment

Comment 8

[Arshad Khan [:arshad]]

> +/* global MozXULElement */

It just tells eslint to MozXULElement is globally defined so dont throw/show errors for that..

Comment 9

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: mail-field.patch (to be used on top of bug 1490269)

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: mail-field.patch (to be used on top of bug 1490269)

Comment 12

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 13

[Arshad Khan [:arshad]]

Comment on attachment 9008685 [details] [diff] [review]
mail-field.patch

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

hey, i instead of makin a separate commit ammended it.. oops.. would this work ? or I create a separate commit.

Comment 14

[Arshad Khan [:arshad]]

Comment on attachment 9008685 [details] [diff] [review]
mail-field.patch

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

hey, i instead of makin a separate commit ammended it.. oops.. would this work ? or I create a separate commit.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9008685 [details] [diff] [review]
mail-field.patch

Separate commit please

Comment 16

[Arshad Khan [:arshad]]

Attachment: mail-field.patch

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9008693 [details] [diff] [review]
mail-field.patch

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

"mail-field" is very generic sounding. 

Maybe better to keep the original names, mail-headerfield, and mail-urlfield. 

Also, why play with the type attribute? I think it would be cleaner to just have the class MozMailfield (well, reaname it MozHeaderfield) and then have another class MozUrlfield that extends MozHeaderfield, like they do at the moment.

::: mail/base/content/mailWidgets.js
@@ +38,5 @@
> +
> +  set headerValue(val) {
> +    let valueNode = this.querySelector(".headerValue");
> +
> +    // Since the control attribute points to the mail-headerfield custom

need to update the comment

Comment 18

[Arshad Khan [:arshad]]

I really dont have very solid opinion about this. The same question I can I ask you, why not play with type attr and save one extra binding?

Comment 19

[Magnus Melin [:mkmelin]]

To better abstract away the difference. That's the reason extends exist in the first place.

And especially mail-headerfield should probably be loaded lazily for performance (using setElementCreationCallback) as it's rather uncommon to use.

Comment 20

[Magnus Melin [:mkmelin]]

eh, meant mail-urlfield of course

Comment 21

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 22

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 23

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 24

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9009062 [details] [diff] [review]
mail-headerfield-urlfield.patch

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

Looks good to me, r=mkmelin with the comma added

::: common/content/customElements.js
@@ +11,4 @@
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  
>  for (let script of [
> +  "chrome://messenger/content/mailWidgets.js"

this needs a comma at the end or eslint will complain about comma-dangle

Comment 26

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 27

[Arshad Khan [:arshad]]

Attachment: mail-headerfield-urlfield.patch

Comment 28

[Jorg K (CEST = GMT+2)]

I assume you want the second patch landed, the one with the r+? Can you mark the first one obsolete then. Was that an alternative approach? - the patch is more than twice as big.

Comment 29

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=676140efeb564897fe506c1639059ff0517e628e

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9008031 [details] [diff] [review]
mail-field.patch (to be used on top of bug 1490269)

(yeah this is obsolete, part of an earlier iteration)

Comment 31

[Jorg K (CEST = GMT+2)]

I cannot land this since it causes a test failure:
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1536929297/build/tests/mozmill/message-header/test-message-header.js | test-message-header.js::test_a11y_attrs

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\message-header\test-message-header.js | test-message-header.js::test_a11y_attrs
  EXCEPTION: element not found for header 'Subject': 'null' == 'null'.
    at: test-folder-display-helpers.js line 2913
       assert_true test-folder-display-helpers.js:2913 11
       assert_not_equals test-folder-display-helpers.js:2907 3
       verify_header_a11y test-message-header.js:269 3
       test_a11y_attrs test-message-header.js:314 3

That can be reproduced locally by running:
make SOLO_TEST=message-header/test-message-header.js mozmill-one

May I suggest that you do a try push yourself in the future before you even submit the patch for review or landing. This will be sufficient:
hg qnew -m "try: -b o -p macosx64,linux64 -u all" try && hg push -f -r tip cc-try && hg qpop && hg qdelete try

Comment 32

[Magnus Melin [:mkmelin]]

Attachment: mail-headerfield-urlfield.patch

Updated, still not working version.

Note the change from mc.a to mc.e (a is for anonymous content - https://searchfox.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#879 which this isn't anymore). 

But the accessibility stuff isn't working. All I get is role 0.
We're using .setAttribute("aria-label", ariaLabel); like before (in xbl), and then we check gAccService.getAccessibleFor at https://searchfox.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js#273

Alexander: any idea of what's wrong? Do you know if setting aria-label for Custom Elements is working in general?

It also seems a bit weird that the role for the the Subject header (and others) would be Ci.nsIAccessibleRole.ROLE_ENTRY since it's not a user changeable field. But that's the way it's been.

Comment 33

[Magnus Melin [:mkmelin]]

Oh, thought I'd check out this in the Accessibility Inspector, for a mail with subject "test" I get

+ pane
  + ...
  + label: "Subject"
    text leaf: "test"
  + nothing
    + entry
      text leaf: "Subject: test"

This looks the same in XBL (without the patch).
"Subject: test" is definitely coming from aria-label.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Magnus Melin [:mkmelin] from comment #32)
> Created attachment 9009565 [details] [diff] [review]
> mail-headerfield-urlfield.patch
> 
> Updated, still not working version.
> 
> Note the change from mc.a to mc.e (a is for anonymous content -
> https://searchfox.org/comm-central/source/mail/test/mozmill/shared-modules/
> test-window-helpers.js#879 which this isn't anymore). 
> 
> But the accessibility stuff isn't working. All I get is role 0.
> We're using .setAttribute("aria-label", ariaLabel); like before (in xbl),
> and then we check gAccService.getAccessibleFor at
> https://searchfox.org/comm-central/source/mail/test/mozmill/message-header/
> test-message-header.js#273
> 
> Alexander: any idea of what's wrong?

aria-label doesn't define accessible role, it makes an element accessible though and provide its accessible name

> Do you know if setting aria-label for
> Custom Elements is working in general?

yes, it should be

> It also seems a bit weird that the role for the the Subject header (and
> others) would be Ci.nsIAccessibleRole.ROLE_ENTRY since it's not a user
> changeable field. But that's the way it's been.

what role is more appropriate you would say? which part of UI this is btw?

(In reply to Magnus Melin [:mkmelin] from comment #33)
> Oh, thought I'd check out this in the Accessibility Inspector, for a mail
> with subject "test" I get
> 
> + pane
>   + ...
>   + label: "Subject"
>     text leaf: "test"
>   + nothing
>     + entry
>       text leaf: "Subject: test"
> 
> This looks the same in XBL (without the patch).
> "Subject: test" is definitely coming from aria-label.

probably not, text leafs are accessible objects created for text nodes, and you thus it cannot have @aria-label.

could you please give me more background info with some pointers, I'm not sure I understand the problem you solve.

Comment 35

[Magnus Melin [:mkmelin]]

(In reply to alexander :surkov (:asurkov) from comment #34)
> aria-label doesn't define accessible role, it makes an element accessible
> though and provide its accessible name

Ok, on the next lines we're actually testing the a11y name too. That doesn't get set properly either. Instead of "Subject: whatever" (from aria-label) we now get just "Subject".

> > It also seems a bit weird that the role for the the Subject header (and
> > others) would be Ci.nsIAccessibleRole.ROLE_ENTRY since it's not a user
> > changeable field. But that's the way it's been.
> what role is more appropriate you would say? which part of UI this is btw?

Probably one of ROLE_STATICTEXT, ROLE_LABEL, ROLE_TEXT?

This is in the Thunderbird 3pane, a row in the header of the message being read (not composed). It says what the Subject of the received mail is.

> (In reply to Magnus Melin [:mkmelin] from comment #33)
> > Oh, thought I'd check out this in the Accessibility Inspector, for a mail
> > with subject "test" I get
> > 
> > + pane
> >   + ...
> >   + label: "Subject"
> >     text leaf: "test"
> >   + nothing
> >     + entry
> >       text leaf: "Subject: test"
> > 
> > This looks the same in XBL (without the patch).
> > "Subject: test" is definitely coming from aria-label.
> 
> probably not, text leafs are accessible objects created for text nodes, and thus it cannot have @aria-label.

But it is coming from there. That text doesn't exist elsewhere, and if I remove setting aria-label from the patch, it is no longer there.

> could you please give me more background info with some pointers, I'm not
> sure I understand the problem you solve.

We're converting the mail-headerfield from xbl to a custom element. We had a test for accessibility of the header and that fails for the custom element. Trying to understand why. 

Pointers, well the verify_header_a11y test is at https://searchfox.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js#269: we first find the (now custom element) mail-headerfield and check that the accessibility values are as expected using 
let headerAccessible = nsIAccessibilityService::getAccessibleFor(ourCE); 
let role = headerAccessible.role;
let name = headerAccessible.name;

The role and name are not the same as before. Role isn't set at all, and name is the visible text, not the one from aria-label.

Comment 36

[Magnus Melin [:mkmelin]]

I think if we don't find a solution soon I'll just land the patch with that test disabled, and we can sort it out later. Otherwise this risks blocking other de-xbl work.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Magnus Melin [:mkmelin] from comment #36)
> I think if we don't find a solution soon I'll just land the patch with that
> test disabled, and we can sort it out later. Otherwise this risks blocking
> other de-xbl work.

agreed, might worth to investigate the issue in a separate bug to not block the other work, however it should be kept on track, since there's a risk of regressing a11y.

mail-headerfield XBL widget contains accessible xul:description, which has role_entry because of role="textbox". If you remove role attribute, then xul:description should get a default role_label role.

(In reply to Magnus Melin [:mkmelin] from comment #35)
> > > + pane
> > >   + ...
> > >   + label: "Subject"
> > >     text leaf: "test"
> > >   + nothing
> > >     + entry
> > >       text leaf: "Subject: test"
> > > 
> > > This looks the same in XBL (without the patch).
> > > "Subject: test" is definitely coming from aria-label.
> > 
> > probably not, text leafs are accessible objects created for text nodes, and thus it cannot have @aria-label.
> 
> But it is coming from there. That text doesn't exist elsewhere, and if I
> remove setting aria-label from the patch, it is no longer there.

weird :) perhaps xul:description accessible creates a text leaf accessible (XULLabelTextLeafAccessible), which picks up value from aria-label, but not sure.

> > could you please give me more background info with some pointers, I'm not
> > sure I understand the problem you solve.
> 
> We're converting the mail-headerfield from xbl to a custom element. We had a
> test for accessibility of the header and that fails for the custom element.
> Trying to understand why. 
> 
> Pointers, well the verify_header_a11y test is at
> https://searchfox.org/comm-central/source/mail/test/mozmill/message-header/
> test-message-header.js#269: we first find the (now custom element)
> mail-headerfield and check that the accessibility values are as expected
> using 
> let headerAccessible = nsIAccessibilityService::getAccessibleFor(ourCE); 
> let role = headerAccessible.role;
> let name = headerAccessible.name;

not sure why mail-headerfield element is accessible, maybe it's focusable?, because otherwise it shouldn't be. Then, if I read your patch right, MozMailHeaderfield has no role or aria-label, thus I would expect no accessible role (role nothing if the element is accessible) and no name on it.

> The role and name are not the same as before. Role isn't set at all, and
> name is the visible text, not the one from aria-label.

so technically you need to have only one accessible for mail-headerfield/description bundle, which will define correct role and accessible name. So either mail-headerfield shouldn't be accessible or it should carry proper role/name. You can put aria-label on it for accessible name, but there's no proper @role value afaik (role="textbox", agreed, is a weird one). Is there a way to inherit mail-headerfield CE widget from xul:description element? That would be an elegant solution I guess.

Comment 38

[Magnus Melin [:mkmelin]]

Thanks. I played with this but couldn't make it work.
The good news though is I looked at the implementation more closely and realized the way we intended to convert was a bit nonsensical. We don't need the inner <description> for anything and therefore the whole workaround with setting aria-label is not an issue - accessibility should just work like intended here. The test still doesn't work. But maybe someone can try it out and tell us if there really is anything more we need to do there later.

Comment 39

[Magnus Melin [:mkmelin]]

Attachment: bug1489748_mail-headerfield-urlfield.patch

No more MozXULElement.parseXULToFragment

Comment 40

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=72487739ba0eb0690cf306b714da4bc2eb3d2fbe

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 9010914 [details] [diff] [review]
bug1489748_mail-headerfield-urlfield.patch

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

::: mail/test/mozmill/message-header/test-message-header.js
@@ +70,5 @@
>    // async openings.
>    let contactPanel = mc.eid('editContactPanel').getNode();
>    contactPanel.setAttribute("animate", false);
> +
> +  test_a11y_attrs.__force_skip__ = true; // disabled for now, see 1489748

Hmm, we, well I, usually disable a test by calling it
  function disabled_test_a11y_attrs {
Could that line be places right in front of there test? And please make it ..., see bug 1489748.

Comment 42

[Magnus Melin [:mkmelin]]

Well when you use __force_skip__ mozmill will tell you it skipped that test, so I think that is preferable. I'll add the "bug" keyword.

Comment 43

[Jorg K (CEST = GMT+2)]

Agreed. I just didn't know better. All the ones I had disabled like this are now re-enabled, bar one:
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mail/test/mozmill/migration-to-rdf-ui-2/test-migrate-to-rdf-ui-2.js#43
Sadly bug 1371898 got stuck somewhere in the cracks. Maybe you guys can settle that one day.

Comment 44

[Magnus Melin [:mkmelin]]

Attachment: bug1489748_mail-headerfield-urlfield.patch

Comment 45

[Magnus Melin [:mkmelin]]

Try looked good too so let's land.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/08bf719c34e9
Migrate mail-headerfield and mail-urlfield to a single custom element. r=arshad

Comment 47

[Jorg K (CEST = GMT+2)]

Filed bug 1493608 as follow-up to re-enable the test.