Closed Bug 1521280 Opened 3 years ago Closed 3 years ago

Remove search-textbox binding

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Duplicate of this bug: 1453666
Priority: -- → P5
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Depends on: 1534799
Blocks: 1534913

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?

Flags: needinfo?(surkov.alexander)

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

Flags: needinfo?(surkov.alexander)

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().

Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf0ba3020f61
support XUL textboxes implemented as Custom Elements using HTML:input in explicit content r=Jamie

(In reply to Pulsebot from comment #11)

Pushed by asurkov@mozilla.com:
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"

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

(In reply to Pulsebot from comment #11)

Pushed by asurkov@mozilla.com:
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.

Keywords: leave-open
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c5798de806e2
Convert search-textbox to a custom element. r=dao,bgrins
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---
Flags: needinfo?(ntim.bugs)

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.

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.

Flags: needinfo?(ntim.bugs)

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.

Attachment #9064516 - Attachment is obsolete: true
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d1dee50991e
Remove special-case that's no longer needed after these patches. r=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).

Flags: needinfo?(sheriffs)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a993bd3862b
Convert search-textbox to a custom element. r=dao,bgrins
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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.

Flags: needinfo?(sheriffs)
Type: enhancement → task
Regressions: 1558574
Regressions: 1584795

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

Regressions: 1606716
Regressions: 1650486
You need to log in before you can comment on or make changes to this bug.