Closed Bug 943047 Opened 7 years ago Closed 7 years ago

Stop <input type=number> from taking focus twice, and enable tabbing backwards through focusable items

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now it seems that <input type=number> taking focus twice (if you tab to the <input type=number> it gets focus, then if you tab again the native anonymous <input type=text> that's in its shadow DOM gets focus).

I can fix this issue by explicitly focusing the anonymous text control if the <input type=number> or any other of its shadow DOM other than the text control is the target of a focus event. However, doing that raises another issue, which is that tabbing _backwards_ gets stuck at the <input type=number>. This turns out to be because the when the anonymous control has focus the nsFocusManager determines that the previous focusable element is the <input type=number> and tries to focus that. Of course the <input type=number> then redirects focus back to the anonymous text control.

Making the <input type=number> tabindex="-1" (or blocking it from getting focus in some other way) is not acceptable since that breaks other things, and content needs to be able to change the tabindex of the <input type=number>. I think we need to teach nsFocusController about form controls that are in the native anonymous content of other controls.
Attached patch patch (obsolete) — Splinter Review
Attachment #8338005 - Flags: review?(bugs)
Comment on attachment 8338005 [details] [diff] [review]
patch

So why type="file" works? 

Also, type="file" handles tab index forwarding. I think type="number" should too.


Focus changes need tests.
Attachment #8338005 - Flags: review?(bugs)
So basically what's preventing this for file inputs is than nsFocusManager::GetNextTabbableContent skips over the nsFileControlFrame because it returns -1 as the tabindex, because HTMLInputElement::IsHTMLFocusable special cases it:

  mozilla::dom::HTMLInputElement::IsHTMLFocusable
  nsGenericHTMLElement::IsFocusableInternal
  nsIContent::IsFocusable
  nsIFrame::IsFocusable
  nsFocusManager::GetNextTabbableContent
  nsFocusManager::DetermineElementToMoveFocus
Attached patch patchSplinter Review
Attachment #8338005 - Attachment is obsolete: true
Attachment #8338179 - Flags: review?(bugs)
The reason for the HTMLInputElement::PreHandleEvent change is because click events can still catch the edge of the <input>, and so the <input> itself can end up being the target of a focus event.
Comment on attachment 8338179 [details] [diff] [review]
patch


>+    if (aVisitor.mEvent->message == NS_FOCUS_CONTENT ||
>+        aVisitor.mEvent->message == NS_BLUR_CONTENT) {
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        if (aVisitor.mEvent->message == NS_FOCUS_CONTENT) {
>+          // Tell our frame it's getting focus so that it can make sure focus
>+          // is moved to our anonymous text control.
>+          nsNumberControlFrame* numberControlFrame =
>+            do_QueryFrame(GetPrimaryFrame());
>+          if (numberControlFrame) {
>+            numberControlFrame->HandleFocusEvent(aVisitor.mEvent);
>+          }
>+        }
>+      }
>+    }
Could you merge this with the existing 
  if (mType == NS_FORM_INPUT_RANGE &&
      (aVisitor.mEvent->message == NS_FOCUS_CONTENT ||
       aVisitor.mEvent->message == NS_BLUR_CONTENT)) {



Need to figure out something better for these controls which take focus in native anon.
But I'll do that in some other bug.
Attachment #8338179 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #6)
> Could you merge this with the existing 
>   if (mType == NS_FORM_INPUT_RANGE &&
>       (aVisitor.mEvent->message == NS_FOCUS_CONTENT ||
>        aVisitor.mEvent->message == NS_BLUR_CONTENT)) {

Meh. Had to undo the addressing of this review comment since this is for NS_FORM_INPUT_RANGE, not NS_FORM_INPUT_NUMBER.

https://hg.mozilla.org/integration/mozilla-inbound/rev/736f56fc70ba

Note that I also am aiming to keep type handling in blocks to make fixing bug 902215 easier.
https://hg.mozilla.org/mozilla-central/rev/35454df149da
https://hg.mozilla.org/mozilla-central/rev/736f56fc70ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: verifyme
removing keyword since input=number will not be on on Firefox 28...
Keywords: verifyme
(In reply to Ioana Budnar, QA [:ioana] from comment #10)
> removing keyword since input=number will not be on on Firefox 28...

I'm at a bit of a loss what to do here. It's obvious this code exists in Firefox 28 (as given by the target milestone) but the feature itself is pref'd off. We need some way to identify a bug as "landed in 28 but shipped in 29". As it stands right now I'm concerned bugs like this will slip through the cracks for testing.
FWIW this has tests and shouldn't really need QA.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.