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

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

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 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35454df149da

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35454df149da
https://hg.mozilla.org/mozilla-central/rev/736f56fc70ba

Comment 10

[Ioana (away)]

removing keyword since input=number will not be on on Firefox 28...

Comment 11

[u279076]

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

Comment 12

[Jonathan Watt [:jwatt]]

FWIW this has tests and shouldn't really need QA.