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

RESOLVED FIXED in mozilla28

Status

()

Core
Keyboard: Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla28
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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" },
(Assignee)

Comment 1

5 years ago
Created attachment 8333626 [details] [diff] [review]
patch
Attachment #8333626 - Flags: review?(masayuki)
(Assignee)

Comment 2

5 years ago
Created attachment 8333859 [details] [diff] [review]
patch

OOps, forgot to qrefresh.
Attachment #8333626 - Attachment is obsolete: true
Attachment #8333626 - Flags: review?(masayuki)
Attachment #8333859 - Flags: review?(masayuki)
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?
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 940698
(Assignee)

Comment 5

5 years ago
Created attachment 8334916 [details] [diff] [review]
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.
Attachment #8333859 - Attachment is obsolete: true
Attachment #8333859 - Flags: review?(masayuki)
Attachment #8334916 - Flags: review?(masayuki)
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.
Attachment #8334916 - Flags: review?(masayuki) → review+
(Assignee)

Comment 7

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/31b5678dbeab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.