1301385 - input type='search' subrole on OS X is wrong

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P5)

Description

[alexander :surkov (:asurkov)]

regression from bug 1137714, see [1]

Accessible::IsSearchField() should check for nsGkAtoms::search value instead nsGkAtoms::textInputType

[1] https://hg.mozilla.org/mozilla-central/rev/5c47eaffd147#l5.17

Comment 1

[Federico Padua [:fedepad]]

(In reply to alexander :surkov from comment #0)
> regression from bug 1137714, see [1]
> 
> Accessible::IsSearchField() should check for nsGkAtoms::search value instead
> nsGkAtoms::textInputType
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/5c47eaffd147#l5.17

Hi, as far as I've seen from the codebase, there is no Accessible::IsSearchField() 
but in the file you point to there is Accessible::IsSearchbox().
In this function you suggest to change nsGkAtoms::textInputType to nsGkAtoms::search. 
But shouldn't be nsGkAtoms::searchbox instead of your suggested nsGkAtoms::search? 
Is this the only change to be made?
If that's the only change to be made I will prepare a patch and push for review.

Thanks in advance for your reply.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Federico Padua (fedepad) from comment #1)
> (In reply to alexander :surkov from comment #0)
> > regression from bug 1137714, see [1]
> > 
> > Accessible::IsSearchField() should check for nsGkAtoms::search value instead
> > nsGkAtoms::textInputType
> > 
> > [1] https://hg.mozilla.org/mozilla-central/rev/5c47eaffd147#l5.17
> 
> Hi, as far as I've seen from the codebase, there is no
> Accessible::IsSearchField() 
> but in the file you point to there is Accessible::IsSearchbox().

correct

> In this function you suggest to change nsGkAtoms::textInputType to
> nsGkAtoms::search. 
> But shouldn't be nsGkAtoms::searchbox instead of your suggested
> nsGkAtoms::search? 

no, the check is supposed to detect input@type="search", see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#Form_%3Cinput%3E_types

> Is this the only change to be made?

yes, we don't have OS X specific tests yet, so no tests for the change

> If that's the only change to be made I will prepare a patch and push for
> review.

thanks!

Comment 3

[Federico Padua [:fedepad]]

Attachment: Bug 1301385 - input type='search' subrole on OS X is wrong;

This patch makes Accessible::IsSearchbox() check for nsGkAtoms::search value
instead of nsGkAtoms::textInputType as noted in the bug description.

Review commit: https://reviewboard.mozilla.org/r/122338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122338/

Comment 4

[Federico Padua [:fedepad]]

Just pushed a patch for review.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8849558 [details]
Bug 1301385 - input type='search' subrole on OS X is wrong;

https://reviewboard.mozilla.org/r/122338/#review124452

r=me, thanks

Comment 6

[Federico Padua [:fedepad]]

Comment on attachment 8849558 [details]
Bug 1301385 - input type='search' subrole on OS X is wrong;

https://reviewboard.mozilla.org/r/122338/#review124452

You're welcome! You can trigger try builds or other stuff because I can't do it!

Comment 7

[Josh Aas]

This bug appears to have a reviewed patch, shame it stalled out. Can someone get this committed?

Comment 8

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbc882ea6aa3

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b39997a18705b9fe8fccd6bb8e29f57ec23d113
Bug 1301385 - input type='search' subrole on OS X is wrong, r=surkov

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7b39997a1870