| Summary: | implement the HTML <main> element | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Steve Faulkner <faulkner.steve> | ||||||||||||||||||||||
| Component: | DOM | Assignee: | sideshowbarker <mike> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, aboxhall, apinheiro, ap, buildbot, cfleizach, cmarcelo, dbates, dglazkov, dmazzoni, eoconnor, ericbidelman, eric, faulkner.steve, ian, jcraig, jdiggs, kennyluck, macpherson, menard, mifenton, mike, mjs, ojan.autocc, ojan, pf, rniwa, silviapf, syoichi, webkit.bugzilla, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| URL: | http://dvcs.w3.org/hg/html-extensions/raw-file/tip/maincontent/index.html | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Steve Faulkner
2012-11-24 02:36:14 PST
once implemented also need to map <main> to main acc role Created attachment 175877 [details]
patch to add <main>
changes made to add main support
mapped LandamrkMainRole to mainTag
[WebCore\accessibility\AccessibilityRenderObject.cpp]
Line 2485 : if (node && node->hasTagName(mainTag))
Line 2486 : return LandmarkMainRole;
added mainTag as a blockTag
[WebCore\WebKit\editing\FormatBlockCommand.cpp]
Line 139 : blockTags.add(mainTag);
added mainTag as a stackItem
[WebCore\WebKit\html\parser\HTMLStackItem.h]
Line 171 : || tagName == HTMLNames::mainTag
added mainTag in tree builder
[WebCore\WebKit\html\parser\HTMLTreeBuilder.cpp]
Line 714 : || token->name() == mainTag
Line 1726 : || token->name() == mainTag
added main to following style declaration:
[WebCore\css\html.css]
article, aside, footer, header, hgroup, main, nav, section {
display: block
}
(In reply to comment #0) > <main> should behave essentially the same as <div> for parsing etc. > > the changes will be the same as for the implementaion of the <nav> element > https://bugs.webkit.org/attachment.cgi?id=35098&action=prettypatch > > implement the HTML <main> element by making the appropriate changes to: > WebCore/css/html.css > WebCore/editing/htmlediting.cpp > WebCore/html/HTMLElement.cpp > WebCore/html/HTMLParser.cpp > WebCore/html/HTMLTagNames.in seems that changes to some of above no longer elevant (In reply to comment #1) > once implemented also need to map <main> to main acc role added main mapping in patch Created attachment 175879 [details]
patch to add <main> v1
fixed formatting
Comment on attachment 175879 [details]
patch to add <main> v1
fixed mimetype
Comment on attachment 175879 [details] patch to add <main> v1 This patch is missing a ChangeLog and tests. Please see http://www.webkit.org/coding/contributing.html for instructions on how to contribute to WebKit. Also, please don't set the review flag to +. That means that a WebKit reviewer has approved your patch. Instead, you should set the review flag to ?, which indicates that you would like a reviewer to look at your patch. My understanding is that this feature is somewhat controversial in the WHATWG. (In reply to comment #7) > (From update of attachment 175879 [details]) > This patch is missing a ChangeLog and tests. Please see http://www.webkit.org/coding/contributing.html for instructions on how to contribute to WebKit. Also, please don't set the review flag to +. That means that a WebKit reviewer has approved your patch. Instead, you should set the review flag to ?, which indicates that you would like a reviewer to look at your patch. ok thanks for the info, first time so unsure of what to do (In reply to comment #8) > My understanding is that this feature is somewhat controversial in the WHATWG. It is only controversial in as much as Hixie does not agree with it. Otherwise it has no major objections and support form a a range of participants. I have provided a patch on the basis of Maciej's email: http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Nov/0104.html > ok thanks for the info, first time so unsure of what to do
We've all contributed for the first time, so we understand. :)
Personally I don't care one way or the other; the reason it's not in the WHATWG spec isn't about my opinion, it's about the lack of compelling use cases. Adding a feature to the platform is expensive. Adding a feature which is likely to be misused (as determined by looking at how people use class=main now, compared to the proposed definition of the element) just pollutes the Web for no good reason. In the case of this patch the danger is even worse; it is likely to actually make accessibility worse. The patch makes <main> act like role=main, but this will cause the many pages that will misuse it to report less useful results to AT users than if the element wasn't there. There are other mechanisms that have been suggested for similar results, which are less likely to be misused. (In reply to comment #12) > Personally I don't care one way or the other; the reason it's not in the WHATWG spec isn't about my opinion, it's about the lack of compelling use cases. Adding a feature to the platform is expensive. Adding a feature which is likely to be misused (as determined by looking at how people use class=main now, compared to the proposed definition of the element) just pollutes the Web for no good reason. > > In the case of this patch the danger is even worse; it is likely to actually make accessibility worse. The patch makes <main> act like role=main, but this will cause the many pages that will misuse it to report less useful results to AT users than if the element wasn't there. There are other mechanisms that have been suggested for similar results, which are less likely to be misused. Making unsubstantiated statements such as much of the above does little to promote your view. What you say are your opinions and not facts, and have not been accepted by others, who have reviewed and commented on the the data, the rationale, uses cases and the spec I wrote and have found them to have merit. Nobody said your data and so forth didn't have merit, just that on the balance you reached the wrong conclusion. (Incidentally, since people on the WHATWG list are encouraged to not post comments along the line of "+1" and since people have seen that I take a relatively reasonable view, people will often just assume that I'm going to reject bad suggestions and not comment. So saying that I'm the only one who has said anything negative on the thread isn't evidence that I'm the only person who reached that conclusion — it's my job to be the one who's negative. You can draw more conclusions from the fact that nobody else is implementing this feature, IMHO. It's not like people don't implement stuff I don't spec — it happens all the time, even without a spec at all. This feature has a spec, even, and still doesn't have implementors. It's not generally a sign of support that the same person who's writing the spec also has to write the code to put it in browsers. Don't confuse indifference for support. But none of this is relevant — what matters isn't how much support something has, it's how much justification it has.) (In reply to comment #14) > Nobody said your data and so forth didn't have merit, just that on the balance you reached the wrong conclusion. > > (Incidentally, since people on the WHATWG list are encouraged to not post comments along the line of "+1" and since people have seen that I take a relatively reasonable view, people will often just assume that I'm going to reject bad suggestions and not comment. So saying that I'm the only one who has said anything negative on the thread isn't evidence that I'm the only person who reached that conclusion — it's my job to be the one who's negative. You can draw more conclusions from the fact that nobody else is implementing this feature, IMHO. It's not like people don't implement stuff I don't spec — it happens all the time, even without a spec at all. This feature has a spec, even, and still doesn't have implementors. It's not generally a sign of support that the same person who's writing the spec also has to write the code to put it in browsers. Don't confuse indifference for support. But none of this is relevant — what matters isn't how much support something has, it's how much justification it has.) The spec has been around a short time, a few months at most, so now you have moved from being in disagreement to being disingenuous , which is unfortunate. The thread started by Simon Pieters on whatwg [1] started with an observation: "My impression from TPAC is that implementors are on board with the idea of adding <main> to HTML, and we're left with Hixie objecting to it." and ended with a specific questions: "If there is anyone besides from Hixie who objects to adding <main>, it would be useful to hear it." While there were many responses there were no other objections. When I tweeted today that I had submitted a patch, the response from Marco Zehe one of the mozilla accessibility engineers: "Want to create a Firefox patch, too?" [2] [1] http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Nov/0055.html [2] https://twitter.com/MarcoInEnglish/status/272648449483218944 In addition to what Adam said about tests and a ChangeLog entry, this is a new feature, so it should be discussed on webkit-dev to determine if the WebKit community agrees with adding it. The details for that are described here: <http://www.webkit.org/coding/adding-features.html>. If there's a need for discussion about whether the <main> feature is a good idea, then please let's have it on the webkit-dev thread, not here in the bug. (In reply to comment #15) > > The spec has been around a short time, a few months at most, so now you have moved from being in disagreement to being disingenuous , which is unfortunate. > The WebKit community has high standards for courteous communication, whether in email or in the bug tracker. Please let's avoid attacks on others' integrity and indeed personal attacks of any kind. (In reply to comment #17) > (In reply to comment #15) > > > > The spec has been around a short time, a few months at most, so now you have moved from being in disagreement to being disingenuous , which is unfortunate. > > > > The WebKit community has high standards for courteous communication, whether in email or in the bug tracker. Please let's avoid attacks on others' integrity and indeed personal attacks of any kind. My apologies for using the word disingenuous in the bug report (In reply to comment #16) > In addition to what Adam said about tests and a ChangeLog entry, this is a new feature, so it should be discussed on webkit-dev to determine if the WebKit community agrees with adding it. The details for that are described here: <http://www.webkit.org/coding/adding-features.html>. > > If there's a need for discussion about whether the <main> feature is a good idea, then please let's have it on the webkit-dev thread, not here in the bug. I have signed up for the mailing list once. I have access will send the appropriate email. Created attachment 175960 [details]
v2 of patch for main - no tests yet
double checked previous changes and new addition to HTMLTagNames.in
With help, got a chromium/Mac build with patch running. Adhoc testing of layout tests https://dl.dropbox.com/u/377471/main2.html all pass and <main> is exposed as AXRole:"AXGroup" AXSubRole:"AXLandmarkMain" AXRoleDescription:"Group" note: role description should be "Main", but Chromium does not appear to expose specific AXRoleDescription for majority of new HTML5 structural elements: <nav> <header> etc, although the AXSubRole values are correct. Comment on attachment 175960 [details]
v2 of patch for main - no tests yet
looks ok.
are you having any trouble making a layout test for accessibility to test this?
The consensus from the webkit-dev discussion on this topic was that this needs further spec-work from the whatwg. Please feel free to file a new bug if this is later if needed. Thanks! http://lists.webkit.org/pipermail/webkit-dev/2012-November/022991.html Created attachment 176418 [details]
complete patch with tests and ChangeLog files
(In reply to comment #23) > The consensus from the webkit-dev discussion on this topic was that this needs further spec-work from the whatwg. > > Please feel free to file a new bug if this is later if needed. > > Thanks! > > http://lists.webkit.org/pipermail/webkit-dev/2012-November/022991.html Seems not unlikely that will be the outcome, but maybe we should give it a few more days, since folks have spoken up on both sides of the issue? (In reply to comment #25) > (In reply to comment #23) > > The consensus from the webkit-dev discussion on this topic was that this needs further spec-work from the whatwg. > > > > Please feel free to file a new bug if this is later if needed. > > > > Thanks! > > > > http://lists.webkit.org/pipermail/webkit-dev/2012-November/022991.html > > Seems not unlikely that will be the outcome, but maybe we should give it a >few more days, since folks have spoken up on both sides of the issue? >The consensus from the webkit-dev discussion on this topic was that this >needs further spec-work from the whatwg. Hi Maciej, some advice here would be useful: Besides the fact that the spec work is not being carried out at the WHATWG. What further spec work needs to go on a the WHATWG? The discussion has been effectively shut down at the WHATWG by hixie i.e. unless new data is provided that convinces him to change his mind. There has been no new data offered or technical argument on the spec on the webkit-dev, only hixies re-assertions that were not backed up with anything new by him and were not convincing when discussed on the WHATWG list. Have I have missed any substantive feedback posted to any lists? So what would you suggest are the next steps? This extension specification was approved "with no objections and ample support" by the main HTML Working Group. http://lists.w3.org/Archives/Public/public-html/2012Nov/0232.html Which means inclusion of the patch should be accepted once it gets reviewer approval. As the standards discussion continues within W3C and WHATWG, additional bugs and patches may need to be added to support future changes to the specification. Comment on attachment 176418 [details]
complete patch with tests and ChangeLog files
we'll still want an accessibility test as well
(In reply to comment #28) > (From update of attachment 176418 [details]) > we'll still want an accessibility test as well I'll contribute one to the patch tomorrow. Reopening to attach new patch. Created attachment 178901 [details]
Patch
Updated patch adds an accessibility layout test. Comment on attachment 178901 [details] Patch Cleared review? from attachment 178901 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). Reopening to attach new patch. Created attachment 182645 [details]
Patch
Reset review? flag per discussion with Chris Fleizach; please hold off on clearing it until Chris has had the opportunity to follow up. Comment on attachment 182645 [details] Patch Attachment 182645 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15870737 New failing tests: editing/execCommand/query-format-block.html fast/dom/wrapper-classes.html (In reply to comment #37) > (From update of attachment 182645 [details]) > Attachment 182645 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15870737 > > New failing tests: > editing/execCommand/query-format-block.html > fast/dom/wrapper-classes.html I'm fixing these now and when I have them passing will upload an updated patch. Comment on attachment 182645 [details] Patch Attachment 182645 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15860756 New failing tests: editing/execCommand/query-format-block.html accessibility/main-element.html Created attachment 182695 [details]
Patch
Fixed failing ests: fixed fast/dom/wrapper-classes.html test for chromium; fixed editing/execCommand/query-format-block.html test for chromium and mac; did not yet fix accessibility/main-element.html test failure on mac -- will follow up with Chris Fleizach to figure out why it's failing.
It looks like your expectation is that the AXRole will be AXLandmarkMain, which will actually be the AXSubrole. You should still get a generic AXGroup for the role. (In reply to comment #41) > It looks like your expectation is that the AXRole will be AXLandmarkMain, which will actually be the AXSubrole. You should still get a generic AXGroup for the role. Thanks James. So I'm wondering if that failure is due to differences in what's exposed by the chromium port vs the mac port. In my chromium build environment, that test passes with the expected result of AXLandmarkMain for AXRole. That's interesting. I'm not sure about the answer then. Check out the rest of that IDL interface in ./Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl If you have a Mac, you can also verify the AX API using the Accessibility Inspector mentioned here: http://developer.apple.com/library/mac/#documentation/Accessibility/Conceptual/AccessibilityMacOSX/OSXAXTesting/OSXAXTestingApps.html (In reply to comment #42) > (In reply to comment #41) > > It looks like your expectation is that the AXRole will be AXLandmarkMain, which will actually be the AXSubrole. You should still get a generic AXGroup for the role. > > Thanks James. So I'm wondering if that failure is due to differences in what's exposed by the chromium port vs the mac port. In my chromium build environment, that test passes with the expected result of AXLandmarkMain for AXRole. Chromium and Mac can have different roles like this. Your test could check the platform name property. You can also look at some of the other landmark tests and see if they do something else Comment on attachment 182695 [details] Patch Attachment 182695 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15884533 New failing tests: accessibility/main-element.html Comment on attachment 182695 [details] Patch Attachment 182695 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15871871 New failing tests: accessibility/main-element.html Comment on attachment 182695 [details] Patch Attachment 182695 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15903335 New failing tests: accessibility/main-element.html Comment on attachment 182695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182695&action=review > Source/WebCore/ChangeLog:1 > +2013-01-14 Steve Faulkner <faulkner.steve@gmail.com> Please change the author to yourself and just mention that it's co-authored by Steve Faulkner in the change log below. Comment on attachment 182695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182695&action=review > LayoutTests/accessibility/main-element.html:19 > + shouldBe("main.role", "'AXRole: AXLandmarkMain'"); what you want here for the mac platform is shouldBe("main.role", "'AXRole: AXGroup'"); shouldBe("main.subrole", "'AXSubrole: AXLandmarkMain'"); Created attachment 183453 [details]
Patch
Removed failing platform-unaware ax test and replaced it with separate platform-specific ax tests (for chromium and mac).
Comment on attachment 183453 [details] Patch Unofficial review, accessibility looks okay with a couple of minor nits. Tests pass so this should be pretty close. View in context: https://bugs.webkit.org/attachment.cgi?id=183453&action=review > LayoutTests/platform/chromium/accessibility/main-element.html:21 > + shouldBe("body.childAtIndex(0).role", "'AXRole: AXLandmarkMain'"); This is okay, but check out accessibilityController.accessibleElementById for a way to do this without needing to assume that the element you want is the first child of the body. (Most of the more recent tests use this pattern instead, it's more robust.) > LayoutTests/platform/mac/accessibility/main-element.html:21 > + shouldBe("body.childAtIndex(0).subrole", "'AXSubrole: AXLandmarkMain'"); This is good, but I would test the role too. Comment on attachment 183453 [details]
Patch
please try to address Dominic's comments. otherwise it's looking good. thanks
Created attachment 183608 [details] Patch Address Dominic's review feedback in comment #51 (use accessibilityController.accessibleElementById and test for both the role and subrole properties). Comment on attachment 183608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183608&action=review > LayoutTests/platform/mac/accessibility/main-element-expected.txt:8 > +PASS main.subrole is 'AXRole: AXLandmarkMain' this line should be 'AXSubrole: AXLandmarkMain' that's prob why its failing Comment on attachment 183608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183608&action=review > LayoutTests/platform/chromium/accessibility/main-element.html:23 > + shouldBe("main.subrole", "undefined"); probably no need to include subrole on chromium since its undefined (In reply to comment #54) > > LayoutTests/platform/mac/accessibility/main-element-expected.txt:8 > > +PASS main.subrole is 'AXRole: AXLandmarkMain' > > this line should be 'AXSubrole: AXLandmarkMain' d'oh > that's prob why its failing yeah will fix it now (In reply to comment #55) > > LayoutTests/platform/chromium/accessibility/main-element.html:23 > > + shouldBe("main.subrole", "undefined"); > > probably no need to include subrole on chromium since its undefined OK, will drop it Created attachment 183611 [details] Patch Fixed test flub and dropped check for ax subrole on chromium per comment #55 from Chris. Comment on attachment 183611 [details] Patch Clearing flags on attachment: 183611 Committed r140341: <http://trac.webkit.org/changeset/140341> All reviewed patches have been landed. Closing bug. Comment on attachment 183611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183611&action=review > LayoutTests/fast/dom/wrapper-classes-expected.txt:346 > +PASS tagJSWrapperClass('main') is 'HTMLElement' > +PASS tagJSWrapperPrototypeClass('main') is 'HTMLElementPrototype' > +PASS tagJSWrapperConstructorClass('main') is 'HTMLElementConstructor' Is this correct? Is this specified? Does this match the behavior in the other browsers? (In reply to comment #60) > (From update of attachment 183611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183611&action=review > > > LayoutTests/fast/dom/wrapper-classes-expected.txt:346 > > +PASS tagJSWrapperClass('main') is 'HTMLElement' > > +PASS tagJSWrapperPrototypeClass('main') is 'HTMLElementPrototype' > > +PASS tagJSWrapperConstructorClass('main') is 'HTMLElementConstructor' > > Is this correct? Is this specified? Does this match the behavior in the other browsers? The quoted results are correct per the HTML 5.1 editor's draft: http://www.w3.org/html/wg/drafts/html/master/single-page.html#the-main-element It doesn't match other browsers because none have implemented this new feature yet, though Gecko engineers plan to implement it, so it will likely soon be in Firefox. |