1521280 - Remove search-textbox binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Tim Nguyen :ntim]

The search-textbox binding powers textbox[type="search"].

Supports:

• A timeout feature
• A clear button
• A search button on Windows/Linux only, Mac has a search icon on the left that does nothing.
• Has custom styling too.

I don't think the interface needs to be ported one to one though.

Possible paths:

• HTML/XUL custom element
• Fix bug 558594 and use input[type=search] or a customized input[type=search] element.
• Drop the special buttons altogether and use a plain/styled input.

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1521280 - Convert search-textbox to a custom element. r=dao,bgrins

Comment 3

[Tim Nguyen :ntim]

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

Comment 4

[Tim Nguyen :ntim]

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

Comment 5

[Tim Nguyen :ntim]

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

Comment 6

[Tim Nguyen :ntim]

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

Comment 7

[Brian Grinstead [:bgrins]]

For https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea86a8336f17cddc42a25c975ee63d0770664e3c&selectedJob=241020138 / TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/states/test_textbox.xul | extraState bits should not be present in ID [ 'input node', address: [object HTMLInputElement] ] [searchfield]!got 'autocompletion', expected '0'

I wonder if the html:input's accessible knows about being in a XBL binding parent and doesn't inherit the aria-autocomplete value being set at https://searchfox.org/mozilla-central/rev/d33d470140ce3f9426af523eaa8ecfa83476c806/toolkit/content/widgets/textbox.xml#304. Both with and without your patch the html:input.textbox-input underneath the content doesn't have any aria attribute and the host element does have aria-autocomplete="list". I would guess the only difference here for a11y is that without the patch the input is XBL anon content. Alex, do you have any idea if this is the case, or pointers on where to look?

Comment 8

[alexander :surkov (:asurkov)]

you're right HTML:input accessible knows weather it is a part of XUL:textbox or not. The state problem related code lives here [1], but there's a whole bunch of other places where HTML:input makes that check. I'd suggest to add a method HTMLTextFieldAccessible::IsInTextbox() which will replace all instances of BindingParent().

[1] https://searchfox.org/mozilla-central/source/accessible/html/HTMLFormControlAccessible.cpp#340

Comment 9

[Brian Grinstead [:bgrins]]

Thanks Alex, that will be helpful to fix for any future de-XBL work for textboxes as well. Just one note is that BindingParent() also returns Native Anonymous Content parent which is used on some form controls so I'm thinking we rename it to BindingOrWidgetParent or similar, and have it also return the closest <xul:textbox>, after checking GetBindingParent().

Comment 10

[alexander :surkov (:asurkov)]

Attachment: Bug 1521280 - support XUL textboxes implemented as Custom Elements using HTML:input in explicit content

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61
support XUL textboxes implemented as Custom Elements using HTML:input in explicit content r=Jamie

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61
support XUL textboxes implemented as Custom Elements using HTML:input in
explicit content r=Jamie

Should this check for the closest textbox check the xul namespace https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61#l2.24? I'm not worried about it if this path is only being hit by chrome code, but if this path could be hit by web content we should probably limit this to xul:textbox to remove false positive.

Note it'd be fine to not handle the case of "find closest xul textbox (skipping any closer non-xul textboxes in the process)" if it makes it easier - but rather "find closest textbox, and if it's xul return it, and if not return null"

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #12)

(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61
support XUL textboxes implemented as Custom Elements using HTML:input in
explicit content r=Jamie

Should this check for the closest textbox check the xul namespace https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61#l2.24? I'm not worried about it if this path is only being hit by chrome code, but if this path could be hit by web content we should probably limit this to xul:textbox to remove false positive.

Agreed, it'd be nicer/cleaner to have the namespace check. Having said that it's very unlikely that textbox tag name is used on the web, but even if it is used and if it contains html:input, and if it defines attributes specific for XUL textboxes, then we'll grab those attributes to alter HTML:input a11y props, but I could hardly imagine that anyone will ever bump into this. HTML:textbox is not part of HTML spec, so if we got a special processing for HTML:input inside of HTML:textbox then as matter of the fact it is not different from processing of HTML:input inside of XUL:textbox, with the exception the former case is a weird one. So I'd say useless but should be harmless.

Note it'd be fine to not handle the case of "find closest xul textbox (skipping any closer non-xul textboxes in the process)" if it makes it easier - but rather "find closest textbox, and if it's xul return it, and if not return null"

XUL:textboxes have more complicated hierarchy than XUL:textbox -> HTML:input, thus I run through parent chain.

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/bf0ba3020f61

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5798de806e2
Convert search-textbox to a custom element. r=dao,bgrins

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c5798de806e2

Comment 17

[Cristina Coroiu [:ccoroiu]]

Backed out changeset c5798de806e2 (bug 1521280) for crashing when searching about:config for upcoming beta (bug 1551013)

Backout: https://hg.mozilla.org/integration/autoland/rev/fa3cfee27619ddc9bcbcf70555bda4eb1e815146

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: Bug 1521280 - Support non-anonymous html:input inside of xul:textbox in nsFocusManager::GetRedirectedFocus

There's a special case for the old about:config search field for SessionStore (CollectFromXULTextbox)
that calls into nsFocusManager::GetRedirectedFocus. Since we are going to make this input field
non-anonymous for xul:textbox[type=search], we need to add support for this case.

Comment 19

[Brian Grinstead [:bgrins]]

So since this made the <html:input> under the <xul:textbox> non-anonymous, the code in nsFocusManager::GetRedirectedFocus started returning null. The reason this was only triggered on beta is because the caller only happens on the old about:config UI (on in beta/release but not nightly). Turns out there's a special case for the old about:config specifically at https://searchfox.org/mozilla-central/rev/6d2cd4ea8b86e19ad16b1dd0b3d736d1e523e854/toolkit/components/sessionstore/SessionStoreUtils.cpp#648-649 that calls into nsFocusManager::GetRedirectedFocus which then has a special case for xul textbox (https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/dom/base/nsFocusManager.cpp#339-341). This looks for an anonymous input inside of the textbox, which doesn't exist anymore.

I've got a patch up that solves it locally, going to ask for review and wait on try results.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1521280 - Remove special-case that's no longer needed after these patches. r=alchen!,bgrins!

After the patches in this bug, the input in the old about:config will no longer
be anonymous, so we don't need this anymore since it'll get handled just fine by
CollectFromInputElement.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d1dee50991e
Remove special-case that's no longer needed after these patches. r=bgrins

Comment 22

[Brian Grinstead [:bgrins]]

Just a heads up (moving this needinfo from https://bugzilla.mozilla.org/show_bug.cgi?id=1551013#c8).

https://hg.mozilla.org/integration/autoland/rev/0d1dee50991e should fix Bug 1551013 (the about:config beta simulation issue). I did a central-as-beta simulation (pushed with ./mach try release -v 68.0b12 --tasks release-sim --migration central-to-beta) at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3740eb94d295a2c710a466c883f790ab2a4a99c0 and everything there seemed unrelated.

The original patch is also queued for autoland (https://lando.services.mozilla.com/D23045/) but waiting for build bustage fix for tree to reopen.

I should have probably landed D23045 first technically, but I don't think it really matters since the old about:config isn't used in m-c builds so this branch isn't tested (hence the beta simulation bustage earlier).

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a993bd3862b
Convert search-textbox to a custom element. r=dao,bgrins

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0d1dee50991e
https://hg.mozilla.org/mozilla-central/rev/5a993bd3862b

Comment 25

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

Beta simulation in comment 22 has no new issues and running one of those DevEdition builds localls doesn't crash searches on about:config anymore.

Comment 26

[Patrick (he/him)]

This bug appears to have caused a regression - https://bugzilla.mozilla.org/show_bug.cgi?id=1606716