Remove namespaced ARIA support in feed parser

RESOLVED FIXED in mozilla14



Disability Access APIs
10 years ago
6 years ago


(Reporter: Aaron Leventhal, Assigned: capella)


(Blocks: 1 bug, {access})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [bk1] [good first bug] [])


(1 attachment, 3 obsolete attachments)



10 years ago
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

10 years ago
Created attachment 292130 [details] [diff] [review]
WIP -- a swipe at it


7 years ago
Blocks: 389800
Someone just needs to update Aaron's patch against trunk and test it.
Whiteboard: [bk1] [good first bug]


6 years ago
Whiteboard: [bk1] [good first bug] → [bk1] [good first bug] []

Comment 3

6 years ago
Created attachment 603639 [details] [diff] [review]
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 ...
Assignee: sayrer → markcapella
Attachment #603639 - Flags: feedback?(marco.zehe)


6 years ago
Attachment #603639 - Flags: feedback?(marco.zehe) → feedback+

Comment 4

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

Asking review from a toolkit peer ...
Attachment #603639 - Flags: review?(robert.bugzilla)

Comment 5

6 years ago
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 on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

Let's try Neil first.
Attachment #603639 - Flags: review?(robert.bugzilla) → review?(enndeakin)
Attachment #603639 - Flags: review?(enndeakin) → review?(mak77)
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).
For example, see^[^\0]*%24&hitlimit=&tree=mozilla-central
Attachment #603639 - Flags: review?(mak77)

Comment 9

6 years ago
Created attachment 606892 [details] [diff] [review]
Re-worked patch (v2)

Ok, I restored the two code portions removed in Aarons original patch ... mochitests all complete locally ...
Mochitests aren't the bulk of feed parser tests - the stuff in is run in make check.

Comment 11

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

Comment 12

6 years ago
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=''") > -1) && (content.text.indexOf("xmlns:xhtml20=''") > -1)); |
Foul Bachelor Frog will tell you exactly what's actually the correct way to deal with those failures, in - they are testing the thing you're removing, so just remove them too. Victory!

Comment 14

6 years ago
Created attachment 606919 [details] [diff] [review]
Re-worked patch (v3)

Lord I hope you weren't pulling my leg :) Anyhow, removing the tests causes ... all to pass !!!
Attachment #603639 - Attachment is obsolete: true
Attachment #606892 - Attachment is obsolete: true


6 years ago
Attachment #606919 - Flags: review?(mak77)
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

6 years ago
thanks for not mocking me mercilessly  :P
Attachment #606919 - Flags: review?(mak77) → review+
Attachment #292130 - Attachment is obsolete: true


6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Last Resolved: 6 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.