Closed Bug 895463 Opened 11 years ago Closed 10 years ago

Orange caret indicator detaches from associated input field

Categories

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

25 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox22 unaffected, firefox23 unaffected, firefox24 ?, firefox25 affected)

RESOLVED FIXED
Tracking Status
firefox22 --- unaffected
firefox23 --- unaffected
firefox24 --- ?
firefox25 --- affected

People

(Reporter: aaronmt, Assigned: capella)

References

()

Details

(Keywords: reproducible)

Attachments

(5 files, 10 obsolete files)

165.92 KB, image/png
Details
1.53 KB, patch
Margaret
: review+
kats
: checkin+
Details | Diff | Splinter Review
3.73 KB, patch
Margaret
: review+
jchen
: review+
Details | Diff | Splinter Review
9.21 KB, patch
kats
: review+
Details | Diff | Splinter Review
13.79 KB, patch
Margaret
: review+
kats
: review+
Details | Diff | Splinter Review
Build: Nightly (07/18)
Devices: HTC One (Android 4.2.2) | Nexus 4 (Android 4.3) | Samsung Galaxy SIV (Android 4.3)

See screenshot

Steps to Reproduce

i) Visit http://duckduckgo.com
ii) Tap the search box

Expected Results

i) Orange caret indicator positioned correctly

Actual Results

i) Orange caret indicator positioned incorrectly when keyboard invokes and page shifts up

Wasn't bug 891175 supposed to fix this?
Bug 891175 fixed the regression from bug 803207. This appears to be a separate issue. (The symptoms are different too - with the bug 803207 regression the caret would have been moved over to the right more than shown in your screenshot).
This is issue is reproducing on Firefox 18, on Firefox 17 there was no orange caret.
Also, I could reproduce this issue only on http://duckduckgo.com. Maybe it has do something with the footer of the page. If you tap again on the text field after keyboard is invoked, you can see the footer appears, and now the orange caret is positioned correctly.
I used STR:

1) Go to DuckDuckGo page
2) Observe cursor blinks in the sites search field, Kbd opens

3) Tap in the sites search field, cursor still blinks, |caret| appears
4) Tap the hardware |back| button
5) Observe Kbd hides, window re-sizes down, cursor still blinks in sites search field, but the |caret| stayed where it was and now "floats" above the page pointing at the duckies head
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch bug895463 (obsolete) — Splinter Review
Another quick fix
Attachment #792512 - Flags: review?(margaret.leibovic)
Attachment #792512 - Attachment is patch: true
Comment on attachment 792512 [details] [diff] [review]
bug895463

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

::: mobile/android/chrome/content/SelectionHandler.js
@@ +85,5 @@
>  
>        case "Window:Resize": {
> +        // Knowing when the page is done drawing is hard, so let's just cancel
> +        // the selection when the window changes. We should fix this later.
> +        this._closeSelection();

With this change, what happens if you tap on an input field to focus it, then the keyboard comes up and changes the size of the window? Won't the caret now disappear in that case? I think that case is one of the reasons we're not doing this now.

See bug 652168 comment 21 for some history.

Ideally, I'd like us to get rid of this "Window:Resize" handler altogether, and actually handle doing the repositioning correctly.
mmmm ... yes the caret disappears vs. being positioned wrong ... so this might be slightly better but (also yes) still incomplete ...
Comment on attachment 792512 [details] [diff] [review]
bug895463

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

I don't like this approach. I think we should try to come up with a solution that correctly repositions the caret, rather than just makes it disappear.
Attachment #792512 - Flags: review?(margaret.leibovic) → review-
Attached patch bug895463 (obsolete) — Splinter Review
Requesting feedback vs review, as I'm still playing with this, but this approach solves this bug for when keyboard opens, closes, and user changes screen orientation.

This might fix or point us in the right direction for a couple of other outstanding bugs that are similar, and maybe help remove the "Window:Resize" part where we just _closeSelection() to hide improperly displayed handles (?)
Attachment #792512 - Attachment is obsolete: true
Attachment #796593 - Flags: feedback?(margaret.leibovic)
Comment on attachment 796593 [details] [diff] [review]
bug895463

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

I think we should ask kats for some feedback about this scrolling stuff, since he knows more about that than I do.
Attachment #796593 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 796593 [details] [diff] [review]
bug895463

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

This seems like a reasonable approach to take.
Attachment #796593 - Flags: feedback?(bugmail.mozilla) → feedback+
Attachment #796593 - Flags: feedback?(margaret.leibovic) → review?(margaret.leibovic)
Attached patch bug895463 v2 (obsolete) — Splinter Review
Rebased ...
Attachment #801124 - Flags: review?(margaret.leibovic)
Attachment #796593 - Flags: review?(margaret.leibovic)
Comment on attachment 801124 [details] [diff] [review]
bug895463 v2

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

This seems to be on the right track. Using duckduckgo.com, if the keyboard is hidden and I tap in the input box, the caret readjusts as expected, but it doesn't readjust when I dismiss the keyboard. Do you know why that's happening?

How often is this MozScrolledAreaChanged event fired? I'm wondering if we could get rid of the timeout or give it a smaller value to reduce the lag for the caret updating itself.

::: mobile/android/chrome/content/browser.js
@@ +3537,5 @@
>          this.updateViewportForPageSize();
> +
> +        // Buffer screen scrolls and update selection handle when stable
> +        if (this._filterScrollTimer)
> +            clearTimeout(this._filterScrollTimer);

Two-space indentation in JS, please. Also add a blank line after this for readability.

@@ +3539,5 @@
> +        // Buffer screen scrolls and update selection handle when stable
> +        if (this._filterScrollTimer)
> +            clearTimeout(this._filterScrollTimer);
> +        this._filterScrollTimer = setTimeout((function() {
> +            this._filterScrollTimer = null;

How about just |delete this._filterScrollTimer|?

@@ +3541,5 @@
> +            clearTimeout(this._filterScrollTimer);
> +        this._filterScrollTimer = setTimeout((function() {
> +            this._filterScrollTimer = null;
> +            SelectionHandler.positionHandles();
> +        }).bind(this), FILTER_SCROLL_TRIGGER);

I don't think you need the extra set of parentheses around the function.
Attachment #801124 - Flags: review?(margaret.leibovic) → feedback+
note to self: http://jsfiddle.net/tEudp/5/
Interesting test case, seems to imply releationship between caret position, and top of softKbd window ...

Follow directions, on loss of element focus I can see the caret reposition down along with the softKbd, though not as a result of messages being generated by SelectionHandler.js nor received by TextSelection.java ...

More thought needed in that direction..
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Attached patch bug895463.diff (obsolete) — Splinter Review
Not sure wy we didn't ever go for something simple like this ... it works for orientation changes, and screen resizes due to keyboard open / close.

I never could find a trigger event / callback for when the resize was finished (even tried mutationObserver) ...

But as this works, maybe its worth putting in?
Attachment #8399723 - Flags: review?(margaret.leibovic)
Attached patch bug895463.diff (v4) (obsolete) — Splinter Review
Sorry for the small mail flood ...

This version addresses an issue specific to TYPE_SELECTION, and fixes a small issue I've been making worse via code copy/paste that leaks otherwise harmless "TypeError: this._contentWindow is null" messages into logcat :(
Assignee: nobody → markcapella
Attachment #779107 - Attachment is obsolete: true
Attachment #796593 - Attachment is obsolete: true
Attachment #801124 - Attachment is obsolete: true
Attachment #8399723 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8399723 - Flags: review?(margaret.leibovic)
Attachment #8399907 - Flags: review?(margaret.leibovic)
Comment on attachment 8399907 [details] [diff] [review]
bug895463.diff (v4)

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

This looks reasonable to me, although I haven't made a build with this patch. I'm wondering if we even need to wait 200ms.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +79,5 @@
>  
>      BrowserApp.deck.removeEventListener("pagehide", this);
> +    BrowserApp.deck.removeEventListener("blur", this, true);
> +    BrowserApp.deck.removeEventListener("scroll", this, true);
> +    BrowserApp.deck.removeEventListener("resize", this, true);

Good catch adding this useCapture param. For consistency, I think you should also add a false parameter to the "pagehide" removeEventListener call.

@@ +993,5 @@
>                   hidden: checkHidden(ex, ey) }];
>      }
>    },
>  
> +  // Wait for reflow before repositioning selection handle/caret

If we're just waiting for a reflow, do we even need to wait 200ms? Would a setTimeout(..., 0) work?
Attachment #8399907 - Flags: review?(margaret.leibovic) → feedback+
:) I tried a setTimeout(,1) during testing to verify my earlier observations that the delay is required ... and found that yes, it was.
The trick was how small to make it of course ... I'll do some more tinkering before I re-post with the nits.
Review flood ! I'll also point out, that I found the delay fairly un-noticeable as visually the keyboard is concurrently rising / lowering, and by the time it arrives the caret / handle snaps to.
(In reply to Mark Capella [:capella] from comment #18)
> :) I tried a setTimeout(,1) during testing to verify my earlier observations
> that the delay is required ... and found that yes, it was.

non-zero delays make me nervous. Is 200ms enough for all phones? Even the armv6 ones? What else could we use instead of a timeout? Some event or notification?
I agree with you there, but the lack of a trigger (as mentioned in comment #15) is what's stymied at least me on this for this long. (Hoping margaret will add in here)

Android InputMethod doesn't seem to provide a callback for onKeyboard(Open/Close)Complete kind-of thing, and neither the re-size event or a new event I tried to generate in GeckoInputConnection on imm.open/close/toggle both seem to occur at start of animation.

I also tinkered with a DOM mutationObserver hoping that it was generated at end-of-event to catch the proper viewport size / element-position but I failed there also.

So instead of suffering an ugly user-visible bug, I thought this, as a mostly-correct and no-worse-than solution, might fly.

Any further thoughts are very welcome :D
Testing on an N7/4.4.2 and GS3/4.3 is fairly consistent. With delay values for POSITION_AFTER_RESIZE_MS less than 200 the patch starts mis-aligning handle/caret positions to worse degrees as the value drops.

The patch works best from around 200 up to 375, then the lag gets unreasonably noticeable.

margaret, when you have a chance to test the build, I wonder if you can try it out on a slow device, as I don't own one, to see if there's an acceptable mid-point that we can agree on?
Flags: needinfo?(margaret.leibovic)
(In reply to Mark Capella [:capella] from comment #23)
> Testing on an N7/4.4.2 and GS3/4.3 is fairly consistent. With delay values
> for POSITION_AFTER_RESIZE_MS less than 200 the patch starts mis-aligning
> handle/caret positions to worse degrees as the value drops.
> 
> The patch works best from around 200 up to 375, then the lag gets
> unreasonably noticeable.
> 
> margaret, when you have a chance to test the build, I wonder if you can try
> it out on a slow device, as I don't own one, to see if there's an acceptable
> mid-point that we can agree on?

Choosing a value based on testing on individual devices makes me uncomfortable. I also don't want to slow down the user experience on fast devices just for the sake of slower devices (especially because the spectrum is really wide, some devices are incredibly slow).

Can we continue to investigate whether there is an event we can listen for? Or if not, can we create a new event, or send a message from Java if we can listen there?

kats, do you know if there's an event that fires when resizing is done?
Flags: needinfo?(margaret.leibovic) → needinfo?(bugmail.mozilla)
We don't have an event for it, no. But I think the place you want to trigger this code from is at the end of the Window:Resize handler in browser.js, right before the break at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=189ae6f118ce#6078 - at that point all the layout stuff that needs updating should be updated, and when you query layout properties as part of moving the selection handles any necessary reflows will be triggered synchronously.
Flags: needinfo?(bugmail.mozilla)
Attached patch bug895463.diff (v5) (obsolete) — Splinter Review
Sending a message from that point to trigger SelectionHandler observer still seems to be acting as if generated |before| the start of the keyboard open/close animations, but that gave me a new direction to backtrack from for clues.

This quick hack into GeckoLayerClient.java does exactly what we need, but I have no idea of the ramifications of impacting processing in that module and that particular routine, and is only offered to illustrate what we're after if I wasn't clear earlier, and as a starting point for further discussion.

I can see several potential issues, the main one being that the routine seems extremely time sensitive and that the issuance of a broadcast event from there may well be a non-starter.

Also, we'd generate more notifications than we really need to receive in SelectionHandler. If this concept isn't already dead, I'd wonder if we could trim those by adding a GeckoEventListener to note Selectionhandler state changes (receiving / not-receiving kinda-thing).
Attachment #8401124 - Flags: feedback?(bugmail.mozilla)
(In reply to Mark Capella [:capella] from comment #26)
> Sending a message from that point to trigger SelectionHandler observer still
> seems to be acting as if generated |before| the start of the keyboard
> open/close animations

By this do you mean that the caret jumped to the final position before the animation, but then it was positioned correctly after the animation finished (i.e. it was no longer "detached from the associated input field")?
The solution as you suggest shows no change over existing/failing behavior:
   https://www.dropbox.com/s/rv9k490ip885rzf/demofail.mp4

The patch I've attached corrects as so:
   https://www.dropbox.com/s/t9ybegzvdl50t4p/demosuccess.mp4
Comment on attachment 8401124 [details] [diff] [review]
bug895463.diff (v5)

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

Ok, in that case I don't think this patch is right, unfortunately. The code you're modifying in GeckoLayerClient runs any time gecko sends a new transaction update to the compositor. In this case you happen to be catching the update you care about after the animation, but it's quite possible to end up catching a previous update for a previous transaction and running this code prematurely. If we add some more conditions to this code to make sure we always pick up the one we want

Have you tried hooking this is in the ScrollTo:FocusedInput listener in browser.js? That one gets invoked when the keyboard appears and if that works we can expand it for keyboard disappearing and rotation and stuff.
Attachment #8401124 - Flags: feedback?(bugmail.mozilla) → feedback-
re: |ScrollTo:FocusedInput| ... Yah, when the first link you suggested didn't help, I backtracked through the train of though to GeckoLayerClient.sendResizeEventIfNecessary() via the Window:Resize messge and found it's emitter there, tracked it down into GeckoAppShell and back over to browser.js to that routine and the ScrollTo:FocusedInput message which looked like it was designed for me!

:( But it doesn't work either.

I'm not sure I read your comment above correctly, are you saying that with some tuning I might be able to hook in there? With the addition of flagging the routine when SelectionListener is able to receive, to limit messages to the important ones, I wasn't seeing any false positioning on my end.
(In reply to Mark Capella [:capella] from comment #30)
> I'm not sure I read your comment above correctly, are you saying that with
> some tuning I might be able to hook in there? With the addition of flagging
> the routine when SelectionListener is able to receive, to limit messages to
> the important ones, I wasn't seeing any false positioning on my end.

Can I see the patch of this? I think that yes, you could hook into the layersUpdated flag but you have to very careful to only pick up the right invocation of that function.
Attached patch bug895463.diff (v6) (obsolete) — Splinter Review
This is the optimized version I've been testing ... it's working nicely with this new approach (not dependent on timer/guess-timations as in previous attempts).

I'll eventually need reviews also from jchen due to renaming of messages I added to GeckoEditable recently to be re-usable in GeckoLayerClient, and of course from margaret for overall SelectionHandler change tracking.

Right now, you're the critical path :-D
Attachment #8399907 - Attachment is obsolete: true
Attachment #8401755 - Flags: review?(bugmail.mozilla)
Comment on attachment 8401755 [details] [diff] [review]
bug895463.diff (v6)

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +719,5 @@
>          }
>  
> +        if (layersUpdated) {
> +            if (mTextSelectionListening && !mTextSelectionDragging) {
> +                /* Used to maintain TextSelection UI */

If I'm understanding the code correctly, this code is going to get hit a lot, not just once when the view resizes. Calling sendEventToGecko is expensive because it incurs a JNI call, and we can't do that in this function because it's running on the compositor thread and will cause us to jank. If it were just getting hit once I could live with it but not if we're going to be hitting it a lot.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +82,4 @@
>  
> +    BrowserApp.deck.removeEventListener("pagehide", this, false);
> +    BrowserApp.deck.removeEventListener("blur", this, true);
> +    BrowserApp.deck.removeEventListener("scroll", this, true);

Please move these unrelated changes into a separate patch
Attachment #8401755 - Flags: review?(bugmail.mozilla) → review-
We don't wind up generating a lot of events there due to the guarding of the trigger ... basically only when there's active text selected and we've signaled our interest in receiving the updates.

The first case where text is selected (anchor != focus / both left and right handles are present) is reasonably small ... we get a small stream of events (5-6) on resize situations.

But yes, in the case of text selection where we display caret only pointing to / tracking the blinking cursor, we get updates due to |layersUpdated| flag as the cursor blinks on ... off ... on ... etc ... regardless of if we're resizing.

I toss those out on the receiving end in SelectionHandler.js, but was hoping next we could find a way to prevent that on the sending side / GeckoLayerClient.java.

Well frack! That will stump me for now. fwiw - I don't see a performance loss or visual jank as you predict, having used the patch for a little while now, but you know a lot more about the critical nature of the code here than I've learned in all of the 4 days I've been tinkering with this new approach.

I had such high hopes again :-(

But!!! ... I seem to be getting closer ... please let me know if you get any ideas ... I'm wondering if instead of messaging into SelectionHandler.js which then messages into TextSelection.java for the actual UI code, I might be able to go straight across bypassing Gecko.

Would that be any better?
Attached patch bug895463p1.diffSplinter Review
Technical fix for eventListener-removal correction as identified earlier
Attachment #8401124 - Attachment is obsolete: true
Attachment #8402176 - Flags: review?(margaret.leibovic)
Would moving where we generate the observer notification over into AsynchCompositionManager on the c++ side make any difference for us?

   http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=8947d2b88791&mark=701-701#648

Instead of c++ -> Java -> JS we'd bypass the bridge?

I'm still trying to figure out how to react to page/layer changes so selection handles can track properly in cases like:

   http://jsfiddle.net/WH5Nj/

Where the user selects text, the handles appear, and then the layout changes ...

Would we still have as critical a timing issue? Is there a better place to hook into?
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8402176 [details] [diff] [review]
bug895463p1.diff

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

It would be best to split this into a different bug, since it's fixing a different problem than the one this bug was filed about, but yes, let's land this fix!
Attachment #8402176 - Flags: review?(margaret.leibovic) → review+
I don't think AsyncCompositionManager is the right place for this stuff, because that deals with compositing and not resizing/layout. I think you were on the right track with your earlier approaches (circa comment 26) and that we need to examine a little closer what all happens on a resize event. All the code that runs on a resize should be getting triggered from Java and JS, so as long as we find the very last thing that happens on a resize, and then reposition the handles after that, it should work just fine. I'm still a little surprised that putting it in the Window:Resize or ScrollTo:FocusedInput handlers didn't work - it would helpful if you could dig into that a bit more and figure out why repositioning the handles there doesn't put them in the right spot.
Flags: needinfo?(bugmail.mozilla)
I posted the fiddle in comment #36 to show that the issue isn't necessarily related to resize events. The duckduckgo.com URL from this bug's original report, and the fiddle both make dynamic DOM changes that don't trigger resizes, but trigger reflows.

In the duckduckgo example, the focused input field does open the keyboard which triggers a resize. But the resize is followed by additional DOM changes due to scripts on that page, and adjusting the handle/caret position on resize is immediately invalidated as a solution.

This is new stuff to me and I may be using terminology imprecisely, but it seemed we need to hook into layer updates.

AsyncCompositionManager::TransformScrollableLayer() is responsible for calling across to GeckoLayerClient.syncViewportInfo() then clearing the |layersUpdated| flag on return. syncViewportInfo() is of course where I had my original hook that you thought having a JNI call would be too performance-adverse.

My new wild idea from comment #36 was to move that logic upstream into that .cpp routine that calls it, in the thought that issuing observer notifications to gecko from there would avoid a bridge call and perhaps be more performant.
(In reply to Mark Capella [:capella] from comment #40)
> I posted the fiddle in comment #36 to show that the issue isn't necessarily
> related to resize events. The duckduckgo.com URL from this bug's original
> report, and the fiddle both make dynamic DOM changes that don't trigger
> resizes, but trigger reflows.

I totally missed this aspect, sorry! If the problem is handles getting out of sync because of arbitrary reflows then yes, the layers update message is probably a reasonable place to handle it.

> AsyncCompositionManager::TransformScrollableLayer() is responsible for
> calling across to GeckoLayerClient.syncViewportInfo() then clearing the
> |layersUpdated| flag on return. syncViewportInfo() is of course where I had
> my original hook that you thought having a JNI call would be too
> performance-adverse.
> 
> My new wild idea from comment #36 was to move that logic upstream into that
> .cpp routine that calls it, in the thought that issuing observer
> notifications to gecko from there would avoid a bridge call and perhaps be
> more performant.

You'd still need a bridge call because the the TransformScrollableLayer code runs on the compositor thread, and scripts run on the gecko thread. So to send a message from TransformScrollableLayer to the script you'd still need to do a message broadcast from AndroidBridge. You'd save on a JNI call, but I think in the grand scheme of things the design cohesion of putting it in syncViewportInfo is better than the performance win. I'll take another look at the patch.
Comment on attachment 8401755 [details] [diff] [review]
bug895463.diff (v6)

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

Ok, so after considering the above I think this general approach makes sense. However rather than shoving text-selection-specific code into GeckoLayerClient, I'd rather just expose the DrawListener interface and move that code into the TextSelection class instead. So what I'd like to see is:

- one patch that exposes DrawListener. This should move the GeckoLayerClient.DrawListener class to LayerView, remove the "robocop only" warnings relating to it, and add methods to LayerView to add and remove draw listeners. GeckoLayerClient will need to store an array of draw listeners instead of just a single one.
- one patch that renames the IMECompositions event to DraggingHandle, assuming you want to keep that rename
- one patch that adds the DraggingHandle listener to TextSelection, along with code that registers/unregisters a DrawListener on LayerView, and fires a message to JS (equivalent to your GeckoLayerClient:LayerUpdate message, but renamed) to do that handling.

Does that sound reasonable? Sorry it's so much more work but I think it will result in more maintainable code in the long run.
Attachment #8401755 - Flags: review- → feedback+
Your approach seems eminently reasonable ... I quite like it :-) Unless my code is very wrong, I don't believe it's a harsh rewrite nor a huge amount of extra work.

(p1) was the bug fix for the listeners in SelectionHandler.

This (p2) I think is along the lines of what you're looking for in the DrawListener exposure ... please point out where my Java might be improved.

I quick pushed it to try, along with the next piece (p3) the dragging message rename. I've also finished the final piece (p4) that satisfies the rest of this bug. I'll post (p3) and (p4) next for reviewer look-aheads, but won't flag until after we wrap-up (p2) and you approve.

tbh, between (p2) and (p3/p4) I'd like to look see if we can do something about the issue I mentioned in comment #34, where superfluous calls to listener.drawFinished(); are generated in GeckoLayerClient.syncViewportInfo() in response to layerUpdates triggered by simple in-place cursor-blinking.

(The actual "|" style text cursor/caret, vs. the orange TextSelection one).
Attachment #8404316 - Flags: review?(bugmail.mozilla)
Comment on attachment 8404316 [details] [diff] [review]
bug895463p2.diff (v0) Expose DrawListener in LayerView

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

This is exactly what I had in mind, except for using the Map to store DrawListener instances. I think it would be better to use a list, and the removeDrawListener function should take the same instance that was passed in addDrawListener. That is a fairly common Java idiom for listeners, and I think it makes sense here. It will avoid subtle errors where the classname being used is not "unique enough" and one piece of code accidentally clobbers the listener for another piece of code (the sort of thing that leads to intermittent failures that are hard to repro/debug).

You'll need to update the PaintExpecter code to keep a reference to the DrawListener instance it creates, and pass that same instance into the removeDrawListener call. I don't think there should be any problems with that.
Attachment #8404316 - Flags: review?(bugmail.mozilla) → feedback+
Comment on attachment 8404320 [details] [diff] [review]
bug895463p4.diff (v0) Reposition on reflow

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

::: mobile/android/base/TextSelection.java
@@ +78,5 @@
>          mMiddleHandle = middleHandle;
>          mEndHandle = endHandle;
>          mEventDispatcher = eventDispatcher;
>  
> +        mLayerView = GeckoAppShell.getLayerView();

It's quite possible for getLayerView() to return null, so it would be better to not store it as an instance variable. Just do the addDrawListener call inside the null guard that was already there in the old code.
Oh, also: in GeckoLayerClient.destroy(), you should clear the list of draw listeners to avoid leaking objects. I can't think of any cases where that would result in undesired behaviour.
Attachment #8404317 - Flags: review?(nchen)
Attachment #8404317 - Flags: review?(margaret.leibovic)
I've been known to over-conceptualize from time to time :-)
Attachment #8401755 - Attachment is obsolete: true
Attachment #8404316 - Attachment is obsolete: true
Attachment #8404788 - Flags: review?(bugmail.mozilla)
Comment on attachment 8404788 [details] [diff] [review]
bug895463p2.diff (v1) Expose DrawListener in LayerView

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

Perfect, thanks!
Attachment #8404788 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8404317 [details] [diff] [review]
bug895463p3.diff (v0) Rename messages

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

LGTM. Consider "TextSelection:OnDragHandle", but it's not a big deal either way.
Attachment #8404317 - Flags: review?(nchen) → review+
Comment on attachment 8404317 [details] [diff] [review]
bug895463p3.diff (v0) Rename messages

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

Or even something like "TextSelection:DraggingHandles", since that matches the _draggingHandles variable. But not a big deal :)
Attachment #8404317 - Flags: review?(margaret.leibovic) → review+
This is the final version of the patch.
Attachment #8404320 - Attachment is obsolete: true
Attachment #8409902 - Flags: review?(margaret.leibovic)
Attachment #8409902 - Flags: review?(bugmail.mozilla)
Attachment #8409902 - Flags: review?(bugmail.mozilla) → review+
Attachment #8409902 - Flags: review?(margaret.leibovic) → review+
Forgot to remove the whiteboard, fixing up ...
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.