Last Comment Bug 691267 - HTML input button and HTML 4 button should share the same class
: HTML input button and HTML 4 button should share the same class
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Andrew Quartey [:drexler]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2011-10-03 01:25 PDT by alexander :surkov
Modified: 2012-02-22 16:02 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.51 KB, patch)
2012-02-12 15:26 PST, Andrew Quartey [:drexler]
hub: review+
roc: review+
Details | Diff | Splinter Review
Patch v2 (16.45 KB, patch)
2012-02-18 13:28 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
Patch v3 (14.84 KB, patch)
2012-02-20 14:59 PST, Andrew Quartey [:drexler]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch v4 (14.70 KB, patch)
2012-02-21 12:52 PST, Andrew Quartey [:drexler]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-10-03 01:25:30 PDT
Classes nsHTMLButtonAccessible (for HTML input button) and nsHTML4ButtonAccessible (for HTML4 button) are quite the same and different by GetNameInternal() only. We could have one class for both of them.
Comment 1 Andrew Quartey [:drexler] 2012-02-12 15:26:39 PST
Created attachment 596524 [details] [diff] [review]
Patch v1
Comment 2 alexander :surkov 2012-02-12 17:39:38 PST
Comment on attachment 596524 [details] [diff] [review]
Patch v1

roc for layout part
Comment 3 Trevor Saunders (:tbsaunde) 2012-02-12 23:41:59 PST
Comment on attachment 596524 [details] [diff] [review]
Patch v1


>   virtual already_AddRefed<nsAccessible>
>-    CreateHTML4ButtonAccessible(nsIContent* aContent, nsIPresShell* aPresShell) = 0;
>-  virtual already_AddRefed<nsAccessible>
>     CreateHTMLButtonAccessible(nsIContent* aContent, nsIPresShell* aPresShell) = 0;

you could make this non-virtual and remove it from nsIAccessibilityService while your here

>   virtual already_AddRefed<nsAccessible>
Comment 4 alexander :surkov 2012-02-13 00:11:24 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> you could make this non-virtual and remove it from nsIAccessibilityService
> while your here

that'd be nice
Comment 5 alexander :surkov 2012-02-13 00:14:35 PST
Technically the logic of nsHTMLButtonAccessible::GetNameInternal isn't applicable to HTML4 buttons. In most cases it works but for example if the user does <button data="something"/> then you get undesired result since @data attribute is not applicable to HTML4 buttons.
Comment 6 alexander :surkov 2012-02-13 00:19:48 PST
also please add states/test_controls.html to test focusable and default states of buttons

also please rename actions/test_inputs.html to actions/test_controls.html and add a test for input@type="button"
Comment 7 alexander :surkov 2012-02-14 17:46:24 PST
Comment on attachment 596524 [details] [diff] [review]
Patch v1

canceling review until comments are addressed
Comment 8 Andrew Quartey [:drexler] 2012-02-18 13:28:34 PST
Created attachment 598573 [details] [diff] [review]
Patch v2

Adjustment per comment 3 and tests for focusable and default states of buttons
Comment 9 alexander :surkov 2012-02-19 06:46:11 PST
Comment on attachment 598573 [details] [diff] [review]
Patch v2

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

1) comment #5 is not addressed
2) use hg rename to rename test_inputs.html to test_controls.html
3) please add disabled button to states/test_button.html and check that focusable state is absent

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +295,5 @@
>  PRUint64
>  nsHTMLButtonAccessible::NativeState()
>  {
>    PRUint64 state = nsHyperTextAccessibleWrap::NativeState();
> +  state |= states::FOCUSABLE;

I don't think you need to set focusable state explicitly
Comment 10 Andrew Quartey [:drexler] 2012-02-20 14:59:35 PST
Created attachment 598980 [details] [diff] [review]
Patch v3
Comment 11 alexander :surkov 2012-02-21 01:14:21 PST
Comment on attachment 598980 [details] [diff] [review]
Patch v3

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

r=me with comments fixed, please upload new patch to land

::: accessible/src/base/nsAccessibilityService.h
@@ +86,5 @@
>    NS_DECL_NSIACCESSIBLERETRIEVAL
>    NS_DECL_NSIOBSERVER
>  
> +  already_AddRefed<nsAccessible>
> +    CreateHTMLButtonAccessible(nsIContent* aContent, nsIPresShell* aPresShell);

I think they were in alphabetical order

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +337,3 @@
>        !mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, name)) {
>      mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::data, name);
>    }

the logic of the whole block is not applicable HTML4 buttons. You should return early like
if (!aName.IsEmpty() || mContent->Tag() != nsGkAtoms::input)

::: accessible/tests/mochitest/actions/test_inputs.html
@@ +72,5 @@
>    <div id="content" style="display: none"></div>
>    <pre id="test">
>    </pre>
>  
> +  <button id="f4_button" value="button">Button</button>

not sure what f4 prefix means here, you can have ids like "button" and "input_button"
what @value attribute is for? afaik it doesn't make any sense on the button

@@ +74,5 @@
>    </pre>
>  
> +  <button id="f4_button" value="button">Button</button>
> +
> +  <input id="norm_button" type="button" value="normal" />

you shouldn't close the single tag since this is HTML (opposite to XHTML)

::: accessible/tests/mochitest/states/test_buttons.html
@@ +19,5 @@
>      // Default state.
>      testStates("f1_image", STATE_DEFAULT);
> +    testStates("f2_submit", STATE_DEFAULT);
> +    testStates("f3_submitbutton", STATE_DEFAULT | STATE_FOCUSABLE);
> +    testStates("f3_disabled_reset", STATE_UNAVAILABLE);

you'd need to check STATE_FOCUSABLE is absent like you do for "f4_disabled_button"
Comment 12 Andrew Quartey [:drexler] 2012-02-21 12:52:48 PST
Created attachment 599311 [details] [diff] [review]
Patch v4
Comment 13 alexander :surkov 2012-02-21 18:56:28 PST
Comment on attachment 599311 [details] [diff] [review]
Patch v4

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

::: accessible/tests/mochitest/states/test_buttons.html
@@ +21,5 @@
> +    testStates("f2_submit", STATE_DEFAULT);
> +    testStates("f3_submitbutton", STATE_DEFAULT | STATE_FOCUSABLE);
> +    testStates("f3_disabled_reset", STATE_UNAVAILABLE, 0, 0, STATE_FOCUSABLE);
> +    testStates("f4_button", STATE_FOCUSABLE, 0, STATE_DEFAULT);
> +    testStates("f4_disabled_button", STATE_UNAVAILABLE, 0, 0, STATE_FOCUSABLE);

absent state_focuable is tested as extra state, I'll fix it prior landing, no new patch is needed
Comment 14 alexander :surkov 2012-02-21 19:13:24 PST
btw, in your patch there are spaces in the end of lines and not unix style line endings, please configure your editor
Comment 16 David Bolter [:davidb] 2012-02-22 11:24:44 PST
Thanks Andrew!
Comment 17 Ed Morley [:emorley] 2012-02-22 16:02:23 PST
https://hg.mozilla.org/mozilla-central/rev/6f1953bd8f46

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