Last Comment Bug 407401 - Remove namespaced ARIA support in feed parser
: Remove namespaced ARIA support in feed parser
[bk1] [good first bug] [mentor=marco....
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
: alexander :surkov
Depends on: 398910
Blocks: cleana11y
  Show dependency treegraph
Reported: 2007-12-07 12:57 PST by Aaron Leventhal
Modified: 2012-03-20 03:55 PDT (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP -- a swipe at it (6.46 KB, patch)
2007-12-07 13:03 PST, Aaron Leventhal
no flags Details | Diff | Splinter Review
Re-worked patch (v1) (8.17 KB, patch)
2012-03-07 01:31 PST, Mark Capella [:capella]
mzehe: feedback+
Details | Diff | Splinter Review
Re-worked patch (v2) (7.81 KB, patch)
2012-03-17 11:56 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Re-worked patch (v3) (11.75 KB, patch)
2012-03-17 18:21 PDT, Mark Capella [:capella]
mak77: review+
Details | Diff | Splinter Review

Description Aaron Leventhal 2007-12-07 12:57:38 PST
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 2007-12-07 13:03:27 PST
Created attachment 292130 [details] [diff] [review]
WIP -- a swipe at it
Comment 2 David Bolter [:davidb] 2011-11-01 14:23:31 PDT
Someone just needs to update Aaron's patch against trunk and test it.
Comment 3 Mark Capella [:capella] 2012-03-07 01:31:11 PST
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 ...
Comment 4 Mark Capella [:capella] 2012-03-08 09:02:32 PST
Comment on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

Asking review from a toolkit peer ...
Comment 5 Mark Capella [:capella] 2012-03-13 05:18:35 PDT
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] 2012-03-13 10:17:52 PDT
Comment on attachment 603639 [details] [diff] [review]
Re-worked patch (v1)

Let's try Neil first.
Comment 7 Marco Bonardo [::mak] 2012-03-14 16:14:19 PDT
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 9 Mark Capella [:capella] 2012-03-17 11:56:56 PDT
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 ...
Comment 10 Phil Ringnalda (:philor) 2012-03-17 12:26:33 PDT
Mochitests aren't the bulk of feed parser tests - the stuff in is run in make check.
Comment 11 Mark Capella [:capella] 2012-03-17 12:39:25 PDT
Ahhh.. ok, new to this area ... how do I perform the tests ... locally? with a TRY push?
Comment 12 Mark Capella [:capella] 2012-03-17 17:50:54 PDT
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)); |
Comment 13 Phil Ringnalda (:philor) 2012-03-17 17:58:40 PDT
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 Mark Capella [:capella] 2012-03-17 18:21:30 PDT
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 !!!
Comment 15 Phil Ringnalda (:philor) 2012-03-17 19:13:59 PDT
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] 2012-03-17 19:16:33 PDT
thanks for not mocking me mercilessly  :P
Comment 18 Mounir Lamouri (:mounir) 2012-03-20 03:55:37 PDT

Note You need to log in before you can comment on or make changes to this bug.