939635 - Make the IME code aware of the particulars of the new <input type=number> implementation

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jonathan Watt [:jwatt]]

With the new <input type=number> support enabled the test test_imestate.html fails with failures like:

   Testing of normal contents: input[type=number], wrong input type - got text,
   expected number

To provide some background, the implementation of <input type=number> now will look like this:

   <input type=number>
     <!-- start of anonymous content -->
     <div>
       <input type=text>
       <div>
         <!-- some button elements -->
       </div>
     </div>
     <!-- end of anonymous content -->
   </input>

The IME code is looking at the focused element when deciding which IME to bring up, and the focused element is the anonymous text control, not the number control. The IME code needs to be taught about the implementation of <input type=number> so that it will bring up the correct IME.

As much as a note to myself as anyone else, the failing part of the test is when the test does various checks using:

     { id: "number",
       description: "input[type=number]",
       focusable: !aInDesignMode,
       expectedEnabled: gUtils.IME_STATUS_ENABLED,
       expectedType: "number" },

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

OOps, forgot to qrefresh.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8333859 [details] [diff] [review]
patch

This patch looks like very <input type="number"> sepcific.

I think that aContent shouldn't be referred in

   if (aContent && aContent->GetNameSpaceID() == kNameSpaceID_XHTML &&
       (aContent->Tag() == nsGkAtoms::input ||
        aContent->Tag() == nsGkAtoms::textarea)) {

block.

First of the block, we should check the first non-anonymous content type with nsIContent::FindFirstNonChromeOnlyAccessContent(). If the element is <input> or <textarea>, it should be referred for setting inputType. Otherwise, should use the actual focused <input> or <textarea>. How about you?

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> First of the block, we should check the first non-anonymous content type
> with nsIContent::FindFirstNonChromeOnlyAccessContent(). If the element is
> <input> or <textarea>, it should be referred for setting inputType.
> Otherwise, should use the actual focused <input> or <textarea>. How about
> you?

I'm not sure that FindFirstNonChromeOnlyAccessContent will always work. What about if the <input type=number> is used in chrome? Wouldn't FindFirstNonChromeOnlyAccessContent then look up the parent chain, past the <input type=number>, right up to the root of the chrome document? If so, then that would fail.

I don't really see it as a huge problem that the code very specifically looks at the grandparent to see if it is an input. If we change the structure of the internals of <input type=number> so that that fails then test_imestate.html will catch the problem if we fail to update the IME code. That said, if there's a better way than looking at the grandparent then I'm happy to use it.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Okay, with the patch for bug 940698 we now have HTMLInputElement::GetOwnerNumberControl(). Here's a patch to use that so that the IME code doesn't need to know about the structure of the number control's anonymous shadow tree.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8334916 [details] [diff] [review]
patch

Hmm, I still don't like such specific code, though. I'm not sure why don't you name GetOwnerControl(). If so, it's useful at implementing other new form controls which have similar structure. Then, nobody will need to change here at implementing new input element type.

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31b5678dbeab

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> Comment on attachment 8334916 [details] [diff] [review]
> patch
> 
> Hmm, I still don't like such specific code, though. I'm not sure why don't
> you name GetOwnerControl(). If so, it's useful at implementing other new
> form controls which have similar structure. Then, nobody will need to change
> here at implementing new input element type.

I'd rather change it at the time if we ever encounter any such new controls that need a similar implementation. Right now this behavior is very specific. I understand your point though.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31b5678dbeab