Closed Bug 1067728 Opened 6 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gduan, Assigned: TYLin)

References

Details

(Whiteboard: [ft:system-platform])

Attachments

(7 files, 8 obsolete files)

3.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.63 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.24 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
alive
: review+
Details | Review
As title, gaia should have ability to be informed if the selected text is out of the visible area after scrolling.
Priority: -- → P2
Whiteboard: [ft:system-platform]
Assignee: nobody → tlin
Depends on: 1090008, 1082486
No longer depends on: 1090008
Depends on: 1090008
nsLayoutUtils::IsRectVisibleInScrollFrames() had been used by
TouchCaret. We do the similar check for SelectionCarets.
Attachment #8525931 - Flags: feedback?(mtseng)
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.
Attachment #8525932 - Flags: feedback?(mtseng)
Attachment #8525928 - Flags: feedback?(mtseng) → feedback+
Attachment #8525930 - Flags: feedback?(mtseng) → feedback+
Attachment #8525931 - Flags: feedback?(mtseng) → feedback+
nsLayoutUtils::IsRectVisibleInScrollFrames() had been used by
TouchCaret. We do the similar check for SelectionCarets.
Attachment #8525931 - Attachment is obsolete: true
Attachment #8533098 - Flags: review?(roc)
Attachment #8525932 - Attachment is obsolete: true
Attachment #8533099 - Flags: review?(roc)
Make DispatchSelectionStateChangedEvent() and GetSelectionBoundingRect()
become member functions of SeletionCarets so that they are easier to use
in later patches.
Attachment #8533100 - Flags: review?(roc)
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.
Attachment #8533103 - Flags: superreview?(bugs)
Attachment #8533103 - Flags: review?(roc)
Attachment #8533103 - Flags: feedback?(pchang)
Attachment #8533096 - Attachment description: Part 1 - Generalize scroll frame boundary checking logic. f=mtseng → Part 1 - Generalize scroll frame boundary checking logic. f=mtseng, r=roc (v2)
Attachment #8533098 - Attachment description: Part 2 - Hide start or end selection caret if it's out of scroll frame. f=mtseng, r=roc → Part 2 - Hide start or end selection caret if it's out of scroll frame. f=mtseng, r=roc (v2)
Attachment #8533100 - Attachment description: Part 4 - Refactor two functions in SeletionCarets. r=roc → Part 4 - Refactor two functions in SeletionCarets. r=roc (v1)
Attachment #8533103 - Attachment description: Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug → Part 5 - Dispatch updateposition after scroll end and reflow. r=roc, sr=smaug (v1)
Attachment #8533097 - Attachment description: Part 1.1 - Move IsRectVisibleInScrollFrames to nsLayoutUtils. f=mtseng → Part 1.1 - Move IsRectVisibleInScrollFrames to nsLayoutUtils. f=mtseng, r=roc (v2)
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?
Attachment #8533103 - Flags: feedback?(pchang) → feedback+
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.
Attachment #8533103 - Flags: superreview?(bugs)
Attachment #8533103 - Flags: review?(roc)
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.
Attachment #8533103 - Attachment is obsolete: true
Attachment #8533633 - Flags: superreview?(bugs)
Attachment #8533633 - Flags: review?(roc)
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.
Attachment #8533633 - Flags: superreview?(bugs) → superreview-
Duplicate of this bug: 1063266
Blocks: 1049367
(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?
Attached file WIP for Gaia (obsolete) —
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.
Attachment #8534236 - Flags: superreview?(bugs) → superreview+
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"
Attachment #8533099 - Flags: review?(roc) → review+
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.
Status: NEW → ASSIGNED
Flags: needinfo?(gduan)
Fix a grammar error.
Attachment #8533099 - Attachment is obsolete: true
Attachment #8534790 - Flags: review+
Please help check-in patch 1 ~ 5. We still need to modify Gaia to fix this bug.

Try result in comment 17.
Attached file 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.
Attachment #8534253 - Attachment is obsolete: true
Flags: needinfo?(gduan)
Attachment #8536445 - Flags: review?(alive)
Comment on attachment 8536445 [details] [review]
PR to master

r=me
Attachment #8536445 - Flags: review?(alive) → review+
master: https://github.com/mozilla-b2g/gaia/commit/6aa54357dfc035507125c015ed66e26018908d20
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.