1067728 - [Text Selection] Hide utility bubble after scrolling if the selected text is out of visible area

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[(Inactive after June) George Duan [:gduan] [:喬智]]

As title, gaia should have ability to be informed if the selected text is out of the visible area after scrolling.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1 - Generalize scroll frame boundary checking logic. f=mtseng, r=roc (v1)

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1.1 - Move IsRectVisibleInScrollFrames to nsLayoutUtils. f=mtseng, r=roc (v1)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Hide start or end selection caret if it's out of scroll frame. f=mtseng, r=roc (v1)

nsLayoutUtils::IsRectVisibleInScrollFrames() had been used by
TouchCaret. We do the similar check for SelectionCarets.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 3 - Hide utility bubble if entire selection is not visible. f=mtseng, r=roc (v1)

We cannot tell utility bubble to hide simply because both startFrame and
endFrame are not visible. When select-all is performed in a long scrolling
text area, both frame should be hid, but utility should show.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8525932 [details] [diff] [review]
Part 3 - Hide utility bubble if entire selection is not visible. f=mtseng, r=roc (v1)

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

::: layout/base/SelectionCarets.cpp
@@ +512,5 @@
>  
> +  // If the bounding rect of selection is not visible, send notification to hide
> +  // utility bubble.
> +  nsIFrame* commonAncestorFrame =
> +    nsLayoutUtils::FindNearestCommonAncestorFrame(startFrame, endFrame);

I'm wondering is it works? I take a look at function "FindNearestCommonAncestorFrame" and find that if two frames have same PresContext then it always return root frame. I don't think root frame is what we want in this case. Can you double check for this?

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1 - Generalize scroll frame boundary checking logic. f=mtseng, r=roc (v2)

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1.1 - Move IsRectVisibleInScrollFrames to nsLayoutUtils. f=mtseng, r=roc (v2)

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Hide start or end selection caret if it's out of scroll frame. f=mtseng, r=roc (v2)

nsLayoutUtils::IsRectVisibleInScrollFrames() had been used by
TouchCaret. We do the similar check for SelectionCarets.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 3 - Expose FindNearestCommonAncestorFrame. r=roc (v1)

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 4 - Refactor two functions in SeletionCarets. r=roc (v1)

Make DispatchSelectionStateChangedEvent() and GetSelectionBoundingRect()
become member functions of SeletionCarets so that they are easier to use
in later patches.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v1)

Add a selection state "updateposition" and a field "visible" to indicate
that the current selection's boundingClientRect or visible is changed.
We dispatch this state after scrolling or reflowing is done.

Comment 12

[Peter Chang[:pchang]]

Comment on attachment 8533103 [details] [diff] [review]
Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v1)

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

f=me, if this patch works for selectall case.

::: layout/base/SelectionCarets.cpp
@@ +1213,5 @@
>      UpdateSelectionCarets();
> +
> +    dom::Sequence<SelectionState> state;
> +    state.AppendElement(dom::SelectionState::Updateposition);
> +    DispatchSelectionStateChangedEvent(GetSelection(), state);

Is it possible the mSelectionVisibleInScrollFrames become false after Reflow ishappened?

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8533103 [details] [diff] [review]
Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v1)

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

The selectall seems failed on master. I'm looking into it.

::: layout/base/SelectionCarets.cpp
@@ +1213,5 @@
>      UpdateSelectionCarets();
> +
> +    dom::Sequence<SelectionState> state;
> +    state.AppendElement(dom::SelectionState::Updateposition);
> +    DispatchSelectionStateChangedEvent(GetSelection(), state);

This callback is called after reflowing. I think mSelectionVisibleInScrollFrames should be correct.

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v2)

Add a selection state "updateposition" and a field "visible" to indicate
that the current selection's boundingClientRect or visible is changed.
We dispatch this state after scrolling or reflowing is done.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8533633 [details] [diff] [review]
Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v2)

> 
>+  // If the selection is not visible, we should dispatch a event.
>+  nsIFrame* commonAncestorFrame =
>+    nsLayoutUtils::FindNearestCommonAncestorFrame(startFrame, endFrame);
>+
>+  nsRect selectionRectInRootFrame = GetSelectionBoundingRect(selection);
>+  nsRect selectionRectInCommonAncestorFrame = selectionRectInRootFrame;
>+  nsLayoutUtils::TransformRect(rootFrame, commonAncestorFrame,
>+                               selectionRectInCommonAncestorFrame);
>+
>+  mSelectionVisibleInScrollFrames =
>+    nsLayoutUtils::IsRectVisibleInScrollFrames(commonAncestorFrame,
>+                                               selectionRectInCommonAncestorFrame);
>+  SELECTIONCARETS_LOG("Selection visibility %s",
>+                      (mSelectionVisibleInScrollFrames ? "shown" : "hidden"));
>+
roc needs to review this part.

>+  // SelectionStateChangeEvent should be dispatched before ScrollViewChangeEvent.
Changed

> SelectionCarets::Reflow(DOMHighResTimeStamp aStart, DOMHighResTimeStamp aEnd)
> {
>   if (mVisible) {
>     SELECTIONCARETS_LOG("Update selection carets after reflow!");
>     UpdateSelectionCarets();
>+
>+    DispatchSelectionStateChangedEvent(GetSelection(),
>+                                       SelectionState::Updateposition);
>   }
So this is a worrisome part. How many events do we end up getting here?
Do we really need to dispatch this all the time?
Should we deal with this stuff in a nsARefreshObserver::WillRefresh or something?

Unless proved otherwise, I think the SelectionCarets::Reflow setup is too slow.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v3)

Correct typo.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c29e24d08145

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Olli Pettay [:smaug] from comment #15)

> So this is a worrisome part. How many events do we end up getting here?
> Do we really need to dispatch this all the time?
> Should we deal with this stuff in a nsARefreshObserver::WillRefresh or
> something?
> 
> Unless proved otherwise, I think the SelectionCarets::Reflow setup is too
> slow.

SelectionCarets::Reflow() is called after reflow is done, and the event is only dispatched when selection carets is visible (i.e. mVisible is true). I think we don't need to update selection carets in nsARefreshObserver::WillRefresh. It's sufficient to keep stuff updated as it is.

I've tested some scenarios on Firefox OS on different apps.
1) After scroll ended, Reflow() is called 1 ~ 2 times.
2) After the screen rotated, Reflow() is called 2 ~ 4 times.
3) When dragging the caret, we have many reflow occurs. However, I feel the selection changing is still as quick as it was.

I've tried to avoid dispatching Updateposition if the bounding rect and visibility of the current selection was not changed after reflowing. Unfortunately, the position of the utility bubble will be incorrect after the screen is rotated. Because we add offsets of to the selection rect in shell.js, the rect received on Gaia might be different even if it is not changed in gecko after each reflow. We still need to dispatch the event each time faithfully.

Olli, do you come up with other test cases that might be slow?

Comment 19

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: WIP for Gaia

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

What if there is some js based animation happening while selection carets are visible, something which
causes lots of reflows. Though, I guess since reflows tend to be very slow anyway, maybe it doesn't matter so much.

Ok, fine, I guess it might not be so bad after all.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8533099 [details] [diff] [review]
Part 3 - Expose FindNearestCommonAncestorFrame. r=roc (v1)

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

::: layout/base/nsLayoutUtils.h
@@ +795,5 @@
>    static gfxSize GetTransformToAncestorScale(nsIFrame* aFrame);
>  
>    /**
> +   * Find the nearest common ancestor frame for aFrame1 and aFrame2. The
> +   * ancestor frame could cross-doc.

"could be"

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

George, 

I have a hacky patch attached for Gaia. I need your help to polish it. With my patches landed in gecko, Gaia will receive a selectionstatechanged with a new attribute 'visible' and updated rect before scrollviewchange ended. The scroll offsets in scrollviewchange are no longer needed to calculate the text dialog position. Thanks.

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 3 - Expose FindNearestCommonAncestorFrame. r=roc (v2, carry r+)

Fix a grammar error.

Comment 24

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Please help check-in patch 1 ~ 5. We still need to modify Gaia to fix this bug.

Try result in comment 17.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/712d4d86ffa3
https://hg.mozilla.org/integration/b2g-inbound/rev/396ff5af73a0
https://hg.mozilla.org/integration/b2g-inbound/rev/5e3f405629f0
https://hg.mozilla.org/integration/b2g-inbound/rev/d4eb88301ba7
https://hg.mozilla.org/integration/b2g-inbound/rev/e8fca74aeb6b
https://hg.mozilla.org/integration/b2g-inbound/rev/1ca332cb7a85

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/712d4d86ffa3
https://hg.mozilla.org/mozilla-central/rev/396ff5af73a0
https://hg.mozilla.org/mozilla-central/rev/5e3f405629f0
https://hg.mozilla.org/mozilla-central/rev/d4eb88301ba7
https://hg.mozilla.org/mozilla-central/rev/e8fca74aeb6b
https://hg.mozilla.org/mozilla-central/rev/1ca332cb7a85

Comment 27

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi Alive,
could you review this patch?
We'll have a selectionstatechanged event before scrollviewchange&stopped, and it will have new rect information and also tell us the selection is visible or not.

This patch can be tested in latest pvt build.

Comment 28

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8536445 [details] [review]
PR to master

r=me

Comment 29

[(Inactive after June) George Duan [:gduan] [:喬智]]

master: https://github.com/mozilla-b2g/gaia/commit/6aa54357dfc035507125c015ed66e26018908d20

Comment 30

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.