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

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: drexler)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla13
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 596524 [details] [diff] [review]
Patch v1
Assignee: nobody → andrew.quartey
Attachment #596524 - Flags: review?(surkov.alexander)
(Reporter)

Updated

6 years ago
Attachment #596524 - Flags: review?(hub)
(Reporter)

Comment 2

6 years ago
Comment on attachment 596524 [details] [diff] [review]
Patch v1

roc for layout part
Attachment #596524 - Flags: review?(roc)
Attachment #596524 - Flags: review?(roc) → review+

Updated

6 years ago
Attachment #596524 - Flags: review?(hub) → review+
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>
(Reporter)

Comment 4

6 years ago
(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
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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"
(Reporter)

Comment 7

6 years ago
Comment on attachment 596524 [details] [diff] [review]
Patch v1

canceling review until comments are addressed
Attachment #596524 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

6 years ago
Created attachment 598573 [details] [diff] [review]
Patch v2

Adjustment per comment 3 and tests for focusable and default states of buttons
Attachment #596524 - Attachment is obsolete: true
Attachment #598573 - Flags: review?(surkov.alexander)
(Reporter)

Comment 9

6 years ago
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
(Assignee)

Comment 10

6 years ago
Created attachment 598980 [details] [diff] [review]
Patch v3
Attachment #598573 - Attachment is obsolete: true
Attachment #598573 - Flags: review?(surkov.alexander)
Attachment #598980 - Flags: review?(surkov.alexander)
(Reporter)

Comment 11

6 years ago
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"
Attachment #598980 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 599311 [details] [diff] [review]
Patch v4
Attachment #598980 - Attachment is obsolete: true
Attachment #599311 - Flags: review?(surkov.alexander)
(Reporter)

Comment 13

6 years ago
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
Attachment #599311 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 14

6 years ago
btw, in your patch there are spaces in the end of lines and not unix style line endings, please configure your editor
(Reporter)

Comment 15

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f1953bd8f46
Thanks Andrew!
https://hg.mozilla.org/mozilla-central/rev/6f1953bd8f46
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.