Closed Bug 799094 Opened 7 years ago Closed 7 years ago

Text selection handles are detached when scrolling iframes

Categories

(Firefox for Android :: Text Selection, defect)

15 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected
firefox19 --- verified

People

(Reporter: AdrianT, Assigned: kats)

References

()

Details

Attachments

(4 files, 3 obsolete files)

Attached image 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
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.
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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #671715 - Attachment is obsolete: true
Attachment #671950 - Flags: review?(margaret.leibovic)
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.
Attachment #671955 - Flags: review?(margaret.leibovic)
Assignee: nobody → bugmail.mozilla
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.
Attachment #671949 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #671950 - Flags: review?(margaret.leibovic) → review-
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 :)
Attachment #671955 - Flags: review?(margaret.leibovic) → review+
(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.
Attachment #671950 - Attachment is obsolete: true
Attachment #672832 - Flags: review?(margaret.leibovic)
Added comment as requested, carrying r+
Attachment #672833 - Flags: review+
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.
Attachment #672832 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/842804bae59b
https://hg.mozilla.org/mozilla-central/rev/bc0f08257eda
https://hg.mozilla.org/mozilla-central/rev/2e05402e25b2
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.