1020802 - [Text Selection] Hide utility bubble when user dragging the selectioncarets handle

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

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

Per bug 987040 comment 41, we handle this special case in this bug. Detail use case is described at UX spec[1] page 4 figure 3.

[1] https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8412542

Comment 1

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

Attachment: bug1020802

Comment 2

[(no longer active)]

Comment on attachment 8465139 [details] [diff] [review]
bug1020802

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

I think the right thing to do here is to expose these reason codes <http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionListener.idl#14> through the selectionchange event, and not lie about whether we can cut/copy/paste like this, and just let the app that handles selectionchange decide how they want to deal with various reasons.

Here's a rough sketch for the API:

enum SelectionChangeReason { "drag", "mousedown", "mouseup", ... };

interface SelectionChangeEvent {
  // ...
  SelectionChangeReason? reason; // null means unknown
};

Comment 3

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

Attachment: bug1020802 v2

This patch passing collapsed and reason information to embedder so that embedder could decide what to do base on those information.

Comment 4

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

Attachment: Patch for gaia

Move show/hide bubble logic to gaia.

Comment 5

[(no longer active)]

Comment on attachment 8468327 [details] [diff] [review]
bug1020802 v2

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

::: dom/webidl/SelectionChangeReason.webidl
@@ +12,5 @@
> +  const short mouseup = 4;
> +  const short keypress = 8;
> +  const short selectall = 16;
> +  const short collapsetostart = 32;
> +  const short collapsetoend = 64;

Any reason you did not use a WebIDL enum like I suggested?

(Actually the reason property needs to be a |sequence<SelectionChangeReason> reasons| because we need to be able to present multiple enum values here, so perhaps use an empty array for representing "no reason".)

Comment 6

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Comment on attachment 8468327 [details] [diff] [review]
> bug1020802 v2
> 
> Review of attachment 8468327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/SelectionChangeReason.webidl
> @@ +12,5 @@
> > +  const short mouseup = 4;
> > +  const short keypress = 8;
> > +  const short selectall = 16;
> > +  const short collapsetostart = 32;
> > +  const short collapsetoend = 64;
> 
> Any reason you did not use a WebIDL enum like I suggested?
Beacuse reason is represented by bit flags which means we may have multiple reason. But you suggested can only contains only 1 reason. So I didn't use it. but......
> 
> (Actually the reason property needs to be a |sequence<SelectionChangeReason>
> reasons| because we need to be able to present multiple enum values here, so
> perhaps use an empty array for representing "no reason".)

I didn't know sequence before!!! Thanks a lot. I'll change to WebIDL enum and sequence in my next patch.

Comment 7

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

Attachment: bug1020802 v3

Use enum and sequence as SelectionChangeReason.

Comment 8

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

Attachment: Patch for gaia v2

Reflect to api change.

Comment 9

[(no longer active)]

Comment on attachment 8469857 [details] [diff] [review]
bug1020802 v3

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

::: dom/base/nsGlobalWindow.cpp
@@ +9288,5 @@
>  
> +static bool
> +CheckReason(int16_t aReason, SelectionChangeReason aReasonType)
> +{
> +  switch(aReasonType) {

Nit: space before ( please.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +617,5 @@
>  
>    _selectionChangeHandler: function(e) {
> +    e.stopPropagation();
> +    let boundingClientRect = e.boundingClientRect;
> +    if (!boundingClientRect) {

Hmm, I don't understand why you made this change, please ask someone else to review this part.

::: dom/webidl/SelectionChangeEvent.webidl
@@ +17,4 @@
>  dictionary SelectionChangeEventInit : EventInit {
>    DOMString selectedText = "";
>    DOMRectReadOnly? boundingClientRect = null;
> +  sequence<SelectionChangeReason> reason = [];

Nit: call this |reasons| please.

@@ +24,5 @@
>   ChromeOnly]
>  interface SelectionChangeEvent : Event {
>    readonly attribute DOMString selectedText;
>    readonly attribute DOMRectReadOnly? boundingClientRect;
> +  [Cached, Pure] readonly attribute sequence<SelectionChangeReason> reason;

Nit: call this |reasons| too please.

Comment 10

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #5)
> > Comment on attachment 8468327 [details] [diff] [review]
> > bug1020802 v2
> > 
> > Review of attachment 8468327 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/SelectionChangeReason.webidl
> > @@ +12,5 @@
> > > +  const short mouseup = 4;
> > > +  const short keypress = 8;
> > > +  const short selectall = 16;
> > > +  const short collapsetostart = 32;
> > > +  const short collapsetoend = 64;
> > 
> > Any reason you did not use a WebIDL enum like I suggested?
> Beacuse reason is represented by bit flags which means we may have multiple
> reason. But you suggested can only contains only 1 reason. So I didn't use
> it. but......

For future reference, if I ever say something which doesn't make sense, please say so.  :-)  I sometimes make mistakes like here, and I'd hate for you to waste your time figuring out a way around my mistakes...

Comment 11

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Comment on attachment 8469857 [details] [diff] [review]
> bug1020802 v3
> 
> Review of attachment 8469857 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +617,5 @@
> >  
> >    _selectionChangeHandler: function(e) {
> > +    e.stopPropagation();
> > +    let boundingClientRect = e.boundingClientRect;
> > +    if (!boundingClientRect) {
> 
> Hmm, I don't understand why you made this change, please ask someone else to
> review this part.
> 
Because nsGlobalWindow::UpdateCommands only fill boundingClientRect if we have one range at least. So we might get null boundingClientRect if we don't have any range. So I added check before I use it. Do you think it makes any sense? I think you're best guy to review this part!

Comment 12

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to Morris Tseng [:mtseng] from comment #6)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #5)
> > > Comment on attachment 8468327 [details] [diff] [review]
> > > bug1020802 v2
> > > 
> > > Review of attachment 8468327 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/webidl/SelectionChangeReason.webidl
> > > @@ +12,5 @@
> > > > +  const short mouseup = 4;
> > > > +  const short keypress = 8;
> > > > +  const short selectall = 16;
> > > > +  const short collapsetostart = 32;
> > > > +  const short collapsetoend = 64;
> > > 
> > > Any reason you did not use a WebIDL enum like I suggested?
> > Beacuse reason is represented by bit flags which means we may have multiple
> > reason. But you suggested can only contains only 1 reason. So I didn't use
> > it. but......
> 
> For future reference, if I ever say something which doesn't make sense,
> please say so.  :-)  I sometimes make mistakes like here, and I'd hate for
> you to waste your time figuring out a way around my mistakes...

Sure! I'll discuss with you next time at first. Sorry about it.

Comment 13

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

Attachment: bug1020802 v4 (carry r+: ehsan)

Update to address comment.

Comment 14

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

Attachment: Patch for gaia v3

Reflect to api change again.

Comment 15

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

https://tbpl.mozilla.org/?tree=Try&rev=f25b430d2e26

Comment 16

[(no longer active)]

(In reply to Morris Tseng [:mtseng] from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #9)
> > Comment on attachment 8469857 [details] [diff] [review]
> > bug1020802 v3
> > 
> > Review of attachment 8469857 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/browser-element/BrowserElementChildPreload.js
> > @@ +617,5 @@
> > >  
> > >    _selectionChangeHandler: function(e) {
> > > +    e.stopPropagation();
> > > +    let boundingClientRect = e.boundingClientRect;
> > > +    if (!boundingClientRect) {
> > 
> > Hmm, I don't understand why you made this change, please ask someone else to
> > review this part.
> > 
> Because nsGlobalWindow::UpdateCommands only fill boundingClientRect if we
> have one range at least. So we might get null boundingClientRect if we don't
> have any range. So I added check before I use it. Do you think it makes any
> sense? I think you're best guy to review this part!

Ah I see.  Yes this makes sense then!

Comment 17

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

Comment on attachment 8470607 [details] [diff] [review]
bug1020802 v4 (carry r+: ehsan)

Since webidl changes, I need super-review. Olli, could you review it for me? Thanks.

Comment 18

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

Comment on attachment 8470607 [details] [diff] [review]
bug1020802 v4 (carry r+: ehsan)

This is still ChromeOnly, so fine.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/80999bc444b5

Setting this to leave-open for the Gaia patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80999bc444b5

Comment 21

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

After applying the gaia and the gecko, clicking selectAll in copypaste.html of UI test, the dialog would disappear. 
Still under investigating.

Comment 22

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

Attachment: PR to gaia master

Gaia patch is ready, and waiting for the test result.

Comment 23

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

Comment on attachment 8474249 [details] [review]
PR to gaia master

Hi Alive,
could you review this patch for me?

1. The mainly change is, gecko has added "reasons" in the event of selectionchange and it move out part of logic to gaia.
2. no isTempShortcut/show in detai
3. test updated

Comment 24

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

Thanks,
https://github.com/mozilla-b2g/gaia/commit/86c9735dfd60b21d58207115cdcd5885373969a1