906048 - Defect - On-screen-keyboard covers Facebook's chat boxes

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Comment 1

[Marina Samuel [:emtwo]]

Jim, I've explored this a bit and there are 2 things that I believe are causing this issue so I wanted to get your perspective on them (or anyone else who may be familiar with this code).

1) 'metro_softkeyboard_shown' (ContentAreaObserver.js) and 'click' (Content.js) are fired simultaneously when a textbox is clicked. There seems to be a race condition between both events to access SelectionPrototype._targetElement. If 'click' propagates fast enough to set _targetElement before 'metro_softkeyboard_shown' looks for it, then the browser shifts up as expected. Otherwise it doesn't.

There's a comment in ContentAreaObserver: "This also insures tap events are delivered before we fire this event. Form repositioning requires the selection handler be running before we send this." Which makes me believe that we do intend to send tap events before handling keyboard events but this doesn't work because I think tap events do start first but that doesn't prevent the racing. Perhaps we should have a callback after _targetElement is set for the keyboard event to look at?

2) Facebook seems to be really good at breaking Util.isEditable(). A lot of times it returns false because the click target seems to be some outer div and not the textbox itself. This isn't a a problem with gmail though, so I'm not sure if there's much we can do here except maybe look a few elements deeper for a textbox?

Thanks!

Comment 2

[Jim Mathies [:jimm]]

(In reply to Marina Samuel [:emtwo] from comment #1)
> Jim, I've explored this a bit and there are 2 things that I believe are
> causing this issue so I wanted to get your perspective on them (or anyone
> else who may be familiar with this code).
> 
> 1) 'metro_softkeyboard_shown' (ContentAreaObserver.js) and 'click'
> (Content.js) are fired simultaneously when a textbox is clicked. There seems
> to be a race condition between both events to access
> SelectionPrototype._targetElement. If 'click' propagates fast enough to set
> _targetElement before 'metro_softkeyboard_shown' looks for it, then the
> browser shifts up as expected. Otherwise it doesn't.
> 
> There's a comment in ContentAreaObserver: "This also insures tap events are
> delivered before we fire this event. Form repositioning requires the
> selection handler be running before we send this." Which makes me believe
> that we do intend to send tap events before handling keyboard events but
> this doesn't work because I think tap events do start first but that doesn't
> prevent the racing. Perhaps we should have a callback after _targetElement
> is set for the keyboard event to look at?

Rather than try to sync this up in ContentAreaObserver maybe we could delay the Content:RepositionInfoResponse response to Browser:RepositionInfoRequest for a bit, waiting to see if the selection handler loads up?

> 2) Facebook seems to be really good at breaking Util.isEditable(). A lot of
> times it returns false because the click target seems to be some outer div
> and not the textbox itself. This isn't a a problem with gmail though, so I'm
> not sure if there's much we can do here except maybe look a few elements
> deeper for a textbox?

Yep. You might poke around in android's front end code, they use similar routines and may have run into the same problem.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: Proposed Solution: shift browser up to avoid covering content

This patch aims to shift the browser up whenever the keyboard is up, regardless of whether the target is 'editable'.

I thought this seemed like the best approach because it makes sense that if the keyboard goes up then most likely we've tapped something editable, and either way, we're better off not blocking content with the keyboard, in my opinion (though maybe there's a good reason we don't do this already, not sure). 

I went with this because somehow the keyboard would go up in Facebook but the target would be a giant div which was pretty hard to decipher what made it 'editable'.

Comment 4

[Jim Mathies [:jimm]]

Why did you remove the editable check in _onCaretAttach?

Comment 5

[Marina Samuel [:emtwo]]

Ah, I don't think it needs to be removed. I had removed it earlier because it was resulting in calls to 'closeSelection()', clearing out the target info but I believe the other changes should handle that now since we no longer mind if it's null.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 808706 [details] [diff] [review]
Proposed Solution: shift browser up to avoid covering content

Ok, sounds good with that removed. You probably want to push this to try to get a selection test run.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v2: Shift browser up to avoid covering content

I noticed while working on bug 905808 that this patch also pushes up the browser when any of the chrome textboxes are selected as well which I think we don't want so I added a check for that in ContentAreaObserver but there seems to be a deeper issue which I think is a separate bug.

As I mentioned before, we often misinterpret a clicked textbox on facebook to be some very large div. This seemed to happen with clicking the urlbar as well. Sometimes the target would correctly be the 'urlbar-edit' textbox but sometimes it would unexpectedly be the 'tray' vbox. So this sounds like it may be an issue related to our touch interpretation.

Does it sound ok if we leave the browser shifting when clicking the chrome for now and file this separate issue?

Tests looked good locally and am currently running a try build.

Comment 8

[Marina Samuel [:emtwo]]

Try build looks good I think https://tbpl.mozilla.org/?tree=Try&rev=c4e0c824a9c0

Comment 9

[Jim Mathies [:jimm]]

(In reply to Marina Samuel [:emtwo] from comment #7)
> Created attachment 812896 [details] [diff] [review]
> v2: Shift browser up to avoid covering content
> 
> I noticed while working on bug 905808 that this patch also pushes up the
> browser when any of the chrome textboxes are selected as well which I think
> we don't want so I added a check for that in ContentAreaObserver but there
> seems to be a deeper issue which I think is a separate bug.
> 
> As I mentioned before, we often misinterpret a clicked textbox on facebook
> to be some very large div. This seemed to happen with clicking the urlbar as
> well. Sometimes the target would correctly be the 'urlbar-edit' textbox but
> sometimes it would unexpectedly be the 'tray' vbox. So this sounds like it
> may be an issue related to our touch interpretation.
> 
> Does it sound ok if we leave the browser shifting when clicking the chrome
> for now and file this separate issue?
> 
> Tests looked good locally and am currently running a try build.

Sounds like it. Might be the cause of some of our tap doesn't bring up the keyboard bugs too.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 812896 [details] [diff] [review]
v2: Shift browser up to avoid covering content

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

::: browser/metro/base/content/ContentAreaObserver.js
@@ +231,5 @@
>      }
>  
>      // Request info about the target form element to see if we
>      // need to reposition the browser above the keyboard.
> +    if (!ChromeSelectionHandler._targetElement) {

I think you can use SelectionHelperUI.layerMode here.

Comment 11

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/a3f21b594b6e

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3f21b594b6e

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #16 and found an issue. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-22-03-02-02-mozilla-central/

- Went through the original issue mentioned in the title from comment #0 and ran into some issues
- Had multiple chat windows opened and ensured when the OSK does appear, all the chat windows are moved above the OSK
- Ensured that when the OSK is being displayed, dismissing it will slide the entire window (including the chat boxes) down without any issues
- Went through all of the above test cases using both full & filled views

Issues:

- When tapping on the Facebook Chat box, the OSK doesn't always appear. If you then tap anywhere on the site, the OSK will appear/disappear. Created Bug 929862.