691267 - HTML input button and HTML 4 button should share the same class

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v1

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 596524 [details] [diff] [review]
Patch v1

roc for layout part

Comment 3

[Trevor Saunders (:tbsaunde)]

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 (:asurkov)]

(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 (:asurkov)]

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 (:asurkov)]

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 (:asurkov)]

Comment on attachment 596524 [details] [diff] [review]
Patch v1

canceling review until comments are addressed

Comment 8

[Andrew Quartey [:drexler]]

Attachment: Patch v2

Adjustment per comment 3 and tests for focusable and default states of buttons

Comment 9

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v3

Comment 11

[alexander :surkov (:asurkov)]

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]]

Attachment: Patch v4

Comment 13

[alexander :surkov (:asurkov)]

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 (:asurkov)]

btw, in your patch there are spaces in the end of lines and not unix style line endings, please configure your editor

Comment 15

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f1953bd8f46

Comment 16

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

Thanks Andrew!

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f1953bd8f46