Closed Bug 590779 Opened 14 years ago Closed 14 years ago

Virtual keyboard should not been showed on urlbar first tap

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(2 files, 3 obsolete files)

I have a patch that change http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#2037, but seeing the comment I'm unsure of the implication.

Masayuki, why should we use nsIContent::IME_STATUS_PASSWORD for ime-mode: disabled? The comment says that but I don't have any clue why. Thanks for the help.
Attached patch Patch v0.1 (m-c) (obsolete) — Splinter Review
The patch is just a one line change in nsEditor.cpp to allow ime-mode disabled to be handle correctly into Android.
The reason is that Mac and gtk need different behavior than IME_STATUS_DISABLED. On gtk, simple IME needs on password editor for dead key handling. On Mac, IME_STATUS_PASSWORD changes current keyboard layout forcibly. But IME_STATUS_DISABLE shouldn't do so, at that time, the native key events should be passed to IME.

IME_STATUS_DISABLED is used for non-text-inputtable widgets. E.g., <a>, <button> and document.

IME_STATUS_PASSWORD is used for password editors and editors they have ime-mode: disabled; style.

I wonder why the urlbar shouldn't be able to use IME? Urlbar must be inputtable all languages for international domain and for parameters (%xx is too hard).

And it seems that you want to switch the ime-mode property by some actions, then, you use a keyword "edit". So, I think, cannot you use readonly attribute instead of ime-mode hack?
(In reply to comment #3)

> I wonder why the urlbar shouldn't be able to use IME? Urlbar must be inputtable
> all languages for international domain and for parameters (%xx is too hard).

We do want to use the IME, but we don't want the IME (and the virtual keyboard) to become active on the first "tap" or "click" into the textbox we use for the URL bar.

The problem is screen space. We use a "tap" or "click" on the URL textbox to display the awesomebar right away. The user can pan a list of commonly visited sites right away, without typing anything. However, if the IME (and virtual keyboard) becomes active on the first "tap"/"click" then nearly all the screen space is lost.

We want the first "tap"/"click" to show the awesomebar (we display it via JS from an onclick event). Then, if the user really wants to type, a second "tap"/"click" should be used to active the IME (and virtual keyboard.

Does that make sense? Is there an easier way to make this work?
tracking-fennec: --- → ?
Okay, I don't have any idea of "easier" way. The cause of hacky is that "whether it should open virtual keyboard or not" will have different meaning than IME state.

Ideally, You should add nsIWidget::OpenSoftwareKeyboard() and you should implement it only on mobile widgets. And it should be called from nsIMEStateManager::OnChangeFocus() only when that's needed. nsIMEStateManager can access to focused content's attribute at that time. So, I think that we should define a new attribute which can specify whether the editor open softweare keyboard or not at getting focus. And nsIMEStateManager checks the attribute before it calls nsIWidget::OpenSoftwareKayboard().

And also if the method should be scriptable, you should add another method to nsIDOMWindowUtils or something.

Then, maybe, we can also fix bug 507987 easily.
Attached patch Patch v0.2 (obsolete) — Splinter Review
(In reply to comment #3)
> The reason is that Mac and gtk need different behavior than
> IME_STATUS_DISABLED. On gtk, simple IME needs on password editor for dead key
> handling. On Mac, IME_STATUS_PASSWORD changes current keyboard layout forcibly.
> But IME_STATUS_DISABLE shouldn't do so, at that time, the native key events
> should be passed to IME.
> 
> IME_STATUS_DISABLED is used for non-text-inputtable widgets. E.g., <a>,
> <button> and document.
> 
> IME_STATUS_PASSWORD is used for password editors and editors they have
> ime-mode: disabled; style.
> 
> I wonder why the urlbar shouldn't be able to use IME? Urlbar must be inputtable
> all languages for international domain and for parameters (%xx is too hard).
> 
> And it seems that you want to switch the ime-mode property by some actions,
> then, you use a keyword "edit". So, I think, cannot you use readonly attribute
> instead of ime-mode hack?

Thanks for your explanation and yes using the readonly attribute works like a charm without any platform changes (I assume readonly prevent the element to create a inner editable frame, which in result make us use the nsIContent::IME_MODE_DISABLE flag?)
Assignee: nobody → 21
Attachment #469278 - Attachment is obsolete: true
Attachment #469281 - Attachment is obsolete: true
Attachment #469430 - Flags: review?(mark.finkle)
> Thanks for your explanation and yes using the readonly attribute works like a
> charm without any platform changes (I assume readonly prevent the element to
> create a inner editable frame, which in result make us use the
> nsIContent::IME_MODE_DISABLE flag?)

basically, yes.
Comment on attachment 469430 [details] [diff] [review]
Patch v0.2

* Why not use _edit.readOnly instead of _edit.setAttribute("readonly")
* Don't use #ifdef if the values for Maemo and Android are the same
* urlbar-state -> bcast_urlbarState ? I kinda like knowing it is a broadcaster or at least not a normal XUL widget
Attachment #469430 - Attachment is obsolete: true
Attachment #469469 - Flags: review?(mark.finkle)
Attachment #469430 - Flags: review?(mark.finkle)
tracking-fennec: ? → 2.0b1+
Comment on attachment 469469 [details] [diff] [review]
Patch v0.3 - Address comments

Can we add anything to the browser-chrome tests for this?
Attachment #469469 - Flags: review?(mark.finkle) → review+
Attached patch TestsSplinter Review
Simple tests for checking the readonly state of the field.
I will use this file to add more tests for the awesome panel (navigation into elements mostly, contextmenu, ...) into an other bug
Attachment #469528 - Flags: review?(mark.finkle)
Attachment #469528 - Flags: review?(mark.finkle) → review+
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100826 Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: