407401 - Remove namespaced ARIA support in feed parser

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

ARIA now longer allows namespaced properties.

ARIA looks like this now:
<div role="myrole" aria-[propertyname]="blah">
Just remove any namespaces from the attribute names and values.

Comment 1

[Aaron Leventhal]

Attachment: WIP -- a swipe at it

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Someone just needs to update Aaron's patch against trunk and test it.

Comment 3

[Mark Capella [:capella]]

Attachment: Re-worked patch (v1)

I went ahead and took this ... I did a plug-n-play patch from the previous attachment ... it built clean locally, and mochitest-plain and mochitest-a11y both ran as normal ... what other testing can I do for you? 

I can't push to TRY yet, so maybe that's next ...

Comment 4

[Mark Capella [:capella]]

Comment on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

Asking review from a toolkit peer ...

Comment 5

[Mark Capella [:capella]]

Checking status ... Marco asked me to get a toolkit peer review+ to move this along ... I picked robert at random ... let me know if there's a better way?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

Let's try Neil first.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

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

do we have any kind of test on this that may need updating?

::: toolkit/components/feeds/FeedProcessor.js
@@ -983,1 @@
>                            "='" + attributeValue + "'");

this doesn't apply just to the aria case, a different prefix may still be added (in future), why are we removing it?

@@ -985,5 @@
> -            // write an xmlns declaration if necessary
> -            if (prefix != "xml" && !this._isInScope(uri)) {
> -              this._inScopeNS[this._inScopeNS.length - 1].push(uri);
> -              this._buf += " xmlns:" + prefix + "='" + uri + "'";
> -            }

also this check seems to be more generic than just the rolePrefix stuff you removed above, prefix is assigned by gAllowedXHTMLNamespaces, doesn't look like it should be removed (even if likely it won't be hit atm).

Comment 8

[Marco Bonardo [:mak]]

For example, see http://mxr.mozilla.org/mozilla-central/search?string=wai-&find=feed&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 9

[Mark Capella [:capella]]

Attachment: Re-worked patch (v2)

Ok, I restored the two code portions removed in Aarons original patch ... mochitests all complete locally ...

Comment 10

[Phil Ringnalda (:philor)]

Mochitests aren't the bulk of feed parser tests - the stuff in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/test/xml/ is run in make check.

Comment 11

[Mark Capella [:capella]]

Ahhh.. ok, new to this area ... how do I perform the tests ... locally? with a TRY push?

Comment 12

[Mark Capella [:capella]]

Ok, got help from IRQ ... testing shows two tests failing ... off exploring in that direction ...

FAIL | xml/rfc4287/feed_accessible.xml | Test was: "atom entry with many funky namespaces" | var content = feed.items.queryElementAt(0, Components.interfaces.nsIFeedEntry).content;((content.text.indexOf("h2 aaa:checked") > -1) && (content.text.indexOf("h4 aaa:checked") > -1) && (content.text.indexOf("h6 xml:base") > -1)); |

FAIL | xml/rfc4287/feed_roleatt.xml | Test was: "atom entry with many funky namespaces" | var content = feed.items.queryElementAt(0, Components.interfaces.nsIFeedEntry).content; ((content.text.indexOf("xhtml2:role='wwwwwww") > -1) && (content.text.indexOf("xmlns:wwwwwww='http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#'") > -1) && (content.text.indexOf("xmlns:xhtml20='http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#'") > -1)); |

Comment 13

[Phil Ringnalda (:philor)]

Foul Bachelor Frog will tell you exactly what's actually the correct way to deal with those failures, in http://mozillamemes.tumblr.com/post/19239974320/skip-if-the-first-last-resort-of-the-desperate - they are testing the thing you're removing, so just remove them too. Victory!

Comment 14

[Mark Capella [:capella]]

Attachment: Re-worked patch (v3)

Lord I hope you weren't pulling my leg :) Anyhow, removing the tests causes ... all to pass !!!

Comment 15

[Phil Ringnalda (:philor)]

Exactly right, and figuring out what parts of what I say I actually mean is one of those difficult parts of getting started with Mozilla, like figuring out what failures on tbpl you can and can't ignore.

At least twice, I've stared long and hard at those two tests trying to figure out if there was anything we wanted to salvage from them in a post-ARIA-namespace world, and I've never gotten any closer to figuring out what it would be if there is. Sometimes, if you test multiple things while saying you are only testing one, and copy around summaries without changing them to say what they really are, and copy around tests that say that they are about one thing when they are something else, you wind up having your tests deleted.

Comment 16

[Mark Capella [:capella]]

thanks for not mocking me mercilessly  :P

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f07eb2670e91

Comment 18

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/f07eb2670e91