799094 - Text selection handles are detached when scrolling iframes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: screenshot

Firefox Mobile 16 RC build 1/ Nightly 18.0a1 2012-10-08
Device: Samsung Galaxy Tab (Android 3.1)

Steps to reproduce:
1. Go to http://w3schools.com/html/html_iframe.asp or any other page that contains text in an iframe.
2. Select a portion of the text.
3. Scroll the iframe.

Expected results:
The selection handles scroll with the selected text.

Actual results:
The selection handles are detached and remain in the original position

Comment 1

[:Margaret Leibovic]

I imagine this is because the code for panning the page (where we currently reposition the handles) is different than the code for panning iframes. We'll probably need to do some JS-Java dance to get these to reposition in iframes instead of being able to do it all in Java.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yeah.. I hadn't considered this case. The JS-Java dance shouldn't be much of a problem, but since the handles are drawn as a separate layer, they're not going to be clipped by the iframe bounds. i.e. if you select a piece of text an in iframe and then scroll the iframe so that the text is not visible in the iframe window, the handles will still be floating over the main document at the point where the iframe text would have been. We'll need to come up with some way to deal with that.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

This makes the handles move in sync with the subdocument that is being scrolled. However it doesn't handle the case where you scroll the selection out of view in the subdocument. The handles still get painted over top of the page even though they should be clipped.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/3) Minor refactoring

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/3) Move handles with iframe scrolling

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/3) Hide handles that go outside the iframe

This needs to be tested more (specifically to make sure it doesn't break normal non-iframe selection, and to verify that nested iframes also work properly) but right now my builds are crashing all over the place so I'm unable to test it more thoroughly. If you have the time please try out these patches.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, I tested the patches a bunch more on a non-crashy build and I couldn't find any problems with them; they seem to work as expected.

Comment 8

[:Margaret Leibovic]

Comment on attachment 671950 [details] [diff] [review]
(2/3) Move handles with iframe scrolling

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

This looks good, but I'm just worried about one case. See below.

::: mobile/android/chrome/content/browser.js
@@ +2073,5 @@
>        }
>      });
> +  },
> +
> +  subdocumentScrolled: function(aElement) {

Nit: Name this function (makes debugging easier if there's an error). sh_subdocumentScrolled with be consistent with our other function names.

@@ +2083,5 @@
> +    while (true) {
> +      if (view == scrollView) {
> +        // The selection is in a view (or sub-view) of the view that scrolled.
> +        // So we need to reposition the handles.
> +        this.updateCacheForSelection();

I think we might run into an error if we call updateCacheForSelection if this._activeType == this.TYPE_CUROSR, since we won't have a valid selection (I don't think we ever call this function when _activeType != TYPE_SELECTION). For the cursor case, it looks like we just get the position in positionHandles, so we can probably just put an |if (this._activeType == this.TYPE_SELECTION)| above the call to updateCacheForSelection. I'm sorry this code became kinda messy with the addition of this cursor thumb logic :/

I haven't build with these patches, but we'd need a testcase with an input box in a scrollable iframe to test this.

Comment 9

[:Margaret Leibovic]

Comment on attachment 671955 [details] [diff] [review]
(3/3) Hide handles that go outside the iframe

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

::: mobile/android/chrome/content/browser.js
@@ +2050,5 @@
>      let offset = this._getViewOffset();
>      let scrollX = {}, scrollY = {};
>      this._view.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).getScrollXY(false, scrollX, scrollY);
>  
> +    let checkHidden = function(x, y) {

Add a comment that this function is about scrolled iframes. This text selection code can be pretty messy and confusing, so comments are always helpful :)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/3) Move handles with iframe scrolling

(In reply to Margaret Leibovic [:margaret] from comment #8)
> Nit: Name this function (makes debugging easier if there's an error).
> sh_subdocumentScrolled with be consistent with our other function names.
> 

Fixed.

> > +        // So we need to reposition the handles.
> > +        this.updateCacheForSelection();
> 
> I think we might run into an error if we call updateCacheForSelection if
> this._activeType == this.TYPE_CUROSR, since we won't have a valid selection
> (I don't think we ever call this function when _activeType !=
> TYPE_SELECTION). For the cursor case, it looks like we just get the position
> in positionHandles, so we can probably just put an |if (this._activeType ==
> this.TYPE_SELECTION)| above the call to updateCacheForSelection. I'm sorry
> this code became kinda messy with the addition of this cursor thumb logic :/
> 
> I haven't build with these patches, but we'd need a testcase with an input
> box in a scrollable iframe to test this.

Good catch, I tried that scenario and it did indeed give a ream of errors. Fixed now. The cursor doesn't seem to appear in the right spot though (maybe the same as bug 803075) but it moves consistently with scrolling.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/3) Hide handles that go outside the iframe

Added comment as requested, carrying r+

Comment 12

[:Margaret Leibovic]

Comment on attachment 672832 [details] [diff] [review]
(2/3) Move handles with iframe scrolling

(In reply to Kartikaya Gupta (:kats) from comment #10)

> The cursor doesn't seem to appear in the right spot though (maybe
> the same as bug 803075) but it moves consistently with scrolling.

Thanks for filing additional bugs about the cursor handle. That patch landed pretty hastily, so I wouldn't doubt that there are additional bugs lurking in there.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/842804bae59b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0f08257eda
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e05402e25b2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/842804bae59b
https://hg.mozilla.org/mozilla-central/rev/bc0f08257eda
https://hg.mozilla.org/mozilla-central/rev/2e05402e25b2