Last Comment Bug 648133 - Fire state change event for aria-busy
: Fire state change event for aria-busy
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: 2011ymaila11y
  Show dependency treegraph
 
Reported: 2011-04-06 15:06 PDT by David Bolter [:davidb]
Modified: 2011-07-27 07:50 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (4.94 KB, patch)
2011-04-06 15:06 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
UPDATE PATCH (2.97 KB, patch)
2011-05-24 03:07 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
FIX NITS (3.58 KB, patch)
2011-05-24 04:15 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
PATCH TO LAND (3.58 KB, patch)
2011-05-24 06:06 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description David Bolter [:davidb] 2011-04-06 15:06:19 PDT
Created attachment 524280 [details] [diff] [review]
WIP
Comment 1 alexander :surkov 2011-05-19 23:10:27 PDT
David,
1) please provide correct bug name (we have some implementation of aria-busy already)
2) bug description (no idea what we must do here)
3) any chance to finish the patch?
Comment 2 James Teh [:Jamie] 2011-05-23 06:37:55 PDT
I think this came out of an email thread between Yahoo!, David and I. Since there is a busy state, aria-busy should be exposed using the busy state and a state change event should be fired when it changes.
Comment 3 alexander :surkov 2011-05-23 17:35:45 PDT
Thanks, Jamie. Changing bug summary per comment #2.
Comment 4 Trevor Saunders (:tbsaunde) 2011-05-24 03:07:59 PDT
Created attachment 534701 [details] [diff] [review]
UPDATE PATCH

update patch for trunk and clean up some
Comment 5 alexander :surkov 2011-05-24 03:23:21 PDT
Comment on attachment 534701 [details] [diff] [review]
UPDATE PATCH

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

too many nits I'd like to look at next patch

::: accessible/src/base/nsDocAccessible.cpp
@@ +1037,5 @@
>      return;
>    }
>  
> +  if (aAttribute == nsAccessibilityAtoms::aria_busy) {
> +    PRBool change = !aContent->AttrValueIs(aNameSpaceID, aAttribute,

isOn, isTrue or something more descriptive than change?

@@ +1038,5 @@
>    }
>  
> +  if (aAttribute == nsAccessibilityAtoms::aria_busy) {
> +    PRBool change = !aContent->AttrValueIs(aNameSpaceID, aAttribute,
> +                                         nsAccessibilityAtoms::_true, eCaseMatters);

nit: indent the second line

@@ +1039,5 @@
>  
> +  if (aAttribute == nsAccessibilityAtoms::aria_busy) {
> +    PRBool change = !aContent->AttrValueIs(aNameSpaceID, aAttribute,
> +                                         nsAccessibilityAtoms::_true, eCaseMatters);
> +    nsAccessible* acc = GetAccService()->GetAccessible(aContent);

do GetAccessible(aContent) simply in arguments (no local variable)

::: accessible/tests/mochitest/events/test_aria_statechange.html
@@ +26,5 @@
>       */
>      var gQueue = null;
>  
> +    // Debug stuff.
> +    gA11yEventDumpID = "eventdump";

nit: keep it commented

@@ +45,4 @@
>                     EXT_STATE_EXPANDABLE);
>        };
>  
> +      this.getID = function expand_getID() {

nit: expandNode_getID() please

@@ +49,5 @@
>          return prettyName(aNodeOrID) + " aria-expanded changed";
>        };
>      }
>  
> +    function busyify(aNodeOrID, bBusy)

funny, hope that comes with derivation in english :)

bBusy is also funny, but let's keep 'a' for this :)

@@ +64,5 @@
> +                   (bBusy ? 0 : STATE_BUSY), 0);
> +      };
> +
> +      this.getID = function busyify_getID() {
> +        return prettyName(aNodeOrID) + " aria-busy changed";

please point how it was changed

@@ +77,5 @@
>        gQueue.push(new expandNode("section", false));
>        gQueue.push(new expandNode("div", true));
>        gQueue.push(new expandNode("div", false));
>  
> +      document.getElementById("aria_doc").focus();

what is that, sounds like artifact from David's patch

@@ +107,2 @@
>    <div id="section" role="section" aria-expanded="false">expandable section</div>
>    <div id="div" aria-expanded="false">expandable native div</div>

somewhere here you should add <a> with reference to the bug.
Comment 6 Trevor Saunders (:tbsaunde) 2011-05-24 04:15:04 PDT
Created attachment 534719 [details] [diff] [review]
FIX NITS

HOPEFULLY i GOT ALL THE NITS :)
Comment 7 alexander :surkov 2011-05-24 04:23:31 PDT
Comment on attachment 534719 [details] [diff] [review]
FIX NITS

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

r=me

::: accessible/src/base/nsDocAccessible.cpp
@@ +1038,5 @@
>    }
>  
> +  if (aAttribute == nsAccessibilityAtoms::aria_busy) {
> +    PRBool isOn = !aContent->AttrValueIs(aNameSpaceID, aAttribute,
> +                                           nsAccessibilityAtoms::_true, eCaseMatters);

it'd be great to have right identation :)
Comment 8 Trevor Saunders (:tbsaunde) 2011-05-24 06:06:20 PDT
Created attachment 534746 [details] [diff] [review]
PATCH TO LAND

INDENTATION SHOULD BE CORRECT NOW
Comment 9 Marco Zehe (:MarcoZ) 2011-05-24 07:05:26 PDT
Landed on mozilla-central on Trevor's behalf: http://hg.mozilla.org/mozilla-central/rev/17d9511255e9
Comment 10 Eric Shepherd [:sheppy] 2011-06-17 10:00:59 PDT
The docs already indicate this is what's supposed to happen, so I'm listing it as a bug fix on Firefox 6 for developers.
Comment 11 AndreiD[QA] 2011-07-27 07:50:09 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

All the proposed, reviewed and landed changes in m-c are visible in hg, under beta (i.e. test_aria_changeset.html in http://hg.mozilla.org/releases/mozilla-beta/file/f3e82fad65b2/accessible/tests/mochitest/events )
Setting this as Verified for Firefox 6 Beta.

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