Defect - OSK pushes up content in about:config

RESOLVED FIXED in Firefox 27

Status

Firefox for Metro
Input
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
Firefox 27
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=tbd u=tbd p=3)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
STR:

1) Navigate to about:start
2) tap the search box at the top to open the OSK
3) OSK pushes content up so it's not viewable

Expected result: tapping on any chrome textboxes should not shift the browser

If the search textbox in about:config is not part of the browser chrome this could be the cause of the bug
Comment 0 should refer to about:config rather than about:start.
Summary: OSK pushes up content in about:start → OSK pushes up content in about:config
(Assignee)

Comment 2

5 years ago
p=3
Assignee: nobody → msamuel

Updated

5 years ago
Blocks: 925788
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Summary: OSK pushes up content in about:config → Defect - OSK pushes up content in about:config
Whiteboard: feature=defect c=tbd u=tbd p=3
(Assignee)

Comment 3

5 years ago
Created attachment 817930 [details] [diff] [review]
v1: Workaround to make OSK more consistent

There are a couple of things happening in this patch:

1) Added an observer in SelectionHelperUI so that bindings.xml can use the SelectionHandler instead of ChromeSelectionHandler when needed (about:config)
2) Added code that sends the coordinates of a tap to SelectionHandler so that it can use them to make a better choice about whether or not to shift the browser. This extra check in _calcNewContentPosition() will always be false for chrome selection since we only look at clicks in Content.js (fixes bug 925457 too) and makes browser shift for content selection more accurate.

Tested on about:config, about:start, Facebook

Only remaining problem is tapping on about:config content does not close the keyboard but I believe there is similar code for doing this for about:start that I'll take a look at
Attachment #817930 - Flags: feedback?(jmathies)
(Assignee)

Comment 4

5 years ago
Created attachment 817932 [details] [diff] [review]
v2: Workaround to make OSK more consistent

oops. had a couple of debug statements left in there.
Attachment #817930 - Attachment is obsolete: true
Attachment #817930 - Flags: feedback?(jmathies)
Attachment #817932 - Flags: feedback?(jmathies)

Comment 5

5 years ago
Comment on attachment 817932 [details] [diff] [review]
v2: Workaround to make OSK more consistent

Review of attachment 817932 [details] [diff] [review]:
-----------------------------------------------------------------

minusing only because I'm not sure about all this message passing, generally the save click coords approach looks good.

::: browser/metro/base/content/browser-ui.js
@@ +53,5 @@
>      return document.getElementById(id);
>    });
>  });
>  
> +SelectionHelperUI.addObservers();

Can we call this from within the init method of SelectionHelperUI?

::: browser/metro/base/content/contenthandlers/Content.js
@@ +187,5 @@
> +        // Workaround for bug 925457: we sometimes don't recognize the
> +        // correct tap target or are unable to identify if it's editable.
> +        // Instead always save tap co-ordinates for the keyboard to look for
> +        // when it is up.
> +        sendAsyncMessage("Content:ClickCoords", {

Couldn't we just call SelectionHandler._onClickCoords() directly here and skip all the message bouncing? Looks like:

1) Content.js calls sendAsyncMessage("Content:ClickCoords")
2) BrowserTouchHandler receives this and calls SelectionHelperUI.saveClick()
3) SelectionHelperUI.saveClick() calls _sendAsyncMessage("Browser:ClickCoords")
4) SelectionHandler receives this and saves the coords.

Content.js though has direct access to SelectionHandler.

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +531,5 @@
>  
>        case "Browser:RepositionInfoRequest":
> +        content.setTimeout (function () {
> +          SelectionHandler._repositionInfoRequest(json);
> +        }, 50);

Rather arbitrary timeout here, but I've had to add a couple of these myself. Please add a comment explaining why we delay (and don't tell mbrubeck). :)

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +258,5 @@
>    _selectionMarkIds: [],
>    _targetIsEditable: false,
>  
> +  addObservers: function () {
> +    Services.obs.addObserver(this, "attach_edit_session", false);

Please move these down below the Properties section in this file and give them their own 'Observers' comment header.

Also, you might want to name this observer something like 'attach_edit_session_to_content' or similar.
Attachment #817932 - Flags: feedback?(jmathies) → feedback-
(Assignee)

Comment 6

5 years ago
Created attachment 818584 [details] [diff] [review]
Part 1: v3: Workaround to make OSK more consistent

(In reply to Jim Mathies [:jimm] from comment #5)
> Comment on attachment 817932 [details] [diff] [review]
> v2: Workaround to make OSK more consistent
> 
> Review of attachment 817932 [details] [diff] [review]:
> -----------------------------------------------------------------
> Content.js though has direct access to SelectionHandler.

Oops! Don't know how I missed that. Thanks!

Note that this does not yet fix bug 925457. I have a separate patch for it.
Attachment #817932 - Attachment is obsolete: true
Attachment #818584 - Flags: review?(jmathies)
(Assignee)

Comment 7

5 years ago
Created attachment 818585 [details] [diff] [review]
Part 2: v1: Blur the about:config search box to close the osk when tapping on content
Attachment #818585 - Flags: review?(jmathies)

Comment 8

5 years ago
Comment on attachment 818584 [details] [diff] [review]
Part 1: v3: Workaround to make OSK more consistent

Review of attachment 818584 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/bindings.xml
@@ +199,5 @@
> +                                                event.clientX, event.clientY);
> +          } else {
> +            // If we don't have access to SelectionHelperUI then we are using this
> +            // binding for browser content (e.g. about:config)
> +            Services.obs.notifyObservers(event, "attach_edit_session", "");

nit - "attach_edit_session_to_content"

@@ +298,5 @@
> +                                                  event.clientX, event.clientY);
> +            } else {
> +              // If we don't have access to SelectionHelperUI then we are using this
> +              // binding for browser content (e.g. about:config)
> +              Services.obs.notifyObservers(event, "attach_edit_session", "");

nit - "attach_edit_session_to_content"

::: browser/metro/base/content/browser-ui.js
@@ +111,5 @@
>      PageThumbs.init();
>      NewTabUtils.init();
>      SettingsCharm.init();
>      NavButtonSlider.init();
> +    SelectionHelperUI._init();

I messed up in my original comments. SelectionHelperUI._init() is called when selection is turned on, but I think you want your observer always alive since it calls attacheEditSession, which calls _init. So looks like you need a separate global init() in SelectionHelperUI which registers your observer. I don't think you need to worry about unregistering.

::: browser/metro/base/content/contenthandlers/Content.js
@@ +187,5 @@
> +        // Workaround for bug 925457: we sometimes don't recognize the
> +        // correct tap target or are unable to identify if it's editable.
> +        // Instead always save tap co-ordinates for the keyboard to look for
> +        // when it is up.
> +        SelectionHandler._onClickCoords(aEvent.clientX, aEvent.clientY);

nit - public apis shouldn't have underscores.

Comment 9

5 years ago
Comment on attachment 818585 [details] [diff] [review]
Part 2: v1: Blur the about:config search box to close the osk when tapping on content

Review of attachment 818585 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/config.js
@@ +193,5 @@
> +  _onClick: function () {
> +    // Blur the search box when tapping anywhere else in the content
> +    // in order to close the soft keyboard.
> +    let searchbox = document.getElementById("textbox");
> +    searchbox.blur();

nit - 

document.getElementById("textbox").blur();
Attachment #818585 - Flags: review?(jmathies) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 819227 [details] [diff] [review]
Part 1: v4: Workaround to make OSK more consistent

Addressed comments
Attachment #818584 - Attachment is obsolete: true
Attachment #818584 - Flags: review?(jmathies)
Attachment #819227 - Flags: review?(jmathies)

Updated

5 years ago
Attachment #819227 - Flags: review?(jmathies) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/f826a7afb3cc
https://hg.mozilla.org/integration/fx-team/rev/6567ee93f799
Whiteboard: feature=defect c=tbd u=tbd p=3 → [fixed-in-fx-team]feature=defect c=tbd u=tbd p=3
https://hg.mozilla.org/mozilla-central/rev/f826a7afb3cc
https://hg.mozilla.org/mozilla-central/rev/418d0fbd33b0
https://hg.mozilla.org/mozilla-central/rev/40f8ff3f63b9
https://hg.mozilla.org/mozilla-central/rev/6567ee93f799
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]feature=defect c=tbd u=tbd p=3 → feature=defect c=tbd u=tbd p=3
Target Milestone: --- → Firefox 27
Depends on: 943542
Went through the following defect for iteration #20 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-28-03-02-04-mozilla-central/

- Went through the original test case from comment #0 without any issues
- Taped on the search bar at the top of about:config and ensured the OSK appeared and the content wasn't pushed up
- Taped on several other options that had text boxes as entries and ensured that the OSK appeared without any issues
- Ensured that taping anywhere on the screen dismissed the OSK without any issues
- Ensured that pressing "ESC" dismisses the OSK without any issues
- Taped on several other options and ensured that the content wasn't pushed up making it not visible
- Ensured that all of the above test cases also worked under filled view
You need to log in before you can comment on or make changes to this bug.