Closed Bug 691267 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: drexler)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → andrew.quartey
Attachment #596524 - Flags: review?(surkov.alexander)
Attachment #596524 - Flags: review?(hub)
Comment on attachment 596524 [details] [diff] [review]
Patch v1

roc for layout part
Attachment #596524 - Flags: review?(roc)
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>
(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
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.
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 on attachment 596524 [details] [diff] [review]
Patch v1

canceling review until comments are addressed
Attachment #596524 - Flags: review?(surkov.alexander)
Attached patch Patch v2 (obsolete) — Splinter Review
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)
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
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #598573 - Attachment is obsolete: true
Attachment #598573 - Flags: review?(surkov.alexander)
Attachment #598980 - Flags: review?(surkov.alexander)
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+
Attached patch Patch v4Splinter Review
Attachment #598980 - Attachment is obsolete: true
Attachment #599311 - Flags: review?(surkov.alexander)
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+
btw, in your patch there are spaces in the end of lines and not unix style line endings, please configure your editor
https://hg.mozilla.org/mozilla-central/rev/6f1953bd8f46
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.