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

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

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

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

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 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 on attachment 9008685 [details] [diff] [review] mail-field.patch Separate commit please

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

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?

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.

eh, meant mail-urlfield of course

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

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 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)

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

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.

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.

(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.

(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.

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.

(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.

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.

No more MozXULElement.parseXULToFragment

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.

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.

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.

Try looked good too so let's land.

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

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