Closed Bug 1020802 Opened 6 years ago Closed 5 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

(Whiteboard: [leave open])

Attachments

(3 files, 5 obsolete files)

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
Depends on: 987040
Blocks: 921965
Attached patch bug1020802 (obsolete) — Splinter Review
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Attachment #8465139 - Flags: review?(ehsan)
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
};
Attachment #8465139 - Flags: review?(ehsan) → review-
Attached patch bug1020802 v2 (obsolete) — Splinter Review
This patch passing collapsed and reason information to embedder so that embedder could decide what to do base on those information.
Attachment #8465139 - Attachment is obsolete: true
Attachment #8468327 - Flags: review?(ehsan)
Attached patch Patch for gaia (obsolete) — Splinter Review
Move show/hide bubble logic to gaia.
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".)
Attachment #8468327 - Flags: review?(ehsan) → review-
(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.
Attached patch bug1020802 v3 (obsolete) — Splinter Review
Use enum and sequence as SelectionChangeReason.
Attachment #8468327 - Attachment is obsolete: true
Attachment #8469857 - Flags: review?(ehsan)
Attached patch Patch for gaia v2 (obsolete) — Splinter Review
Reflect to api change.
Attachment #8468987 - Attachment is obsolete: true
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.
Attachment #8469857 - Flags: review?(ehsan) → review+
(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...
(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!
(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.
Update to address comment.
Attachment #8469857 - Attachment is obsolete: true
Reflect to api change again.
Attachment #8469858 - Attachment is obsolete: true
(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 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.
Attachment #8470607 - Flags: superreview?(bugs)
Comment on attachment 8470607 [details] [diff] [review]
bug1020802 v4 (carry r+: ehsan)

This is still ChromeOnly, so fine.
Attachment #8470607 - Flags: superreview?(bugs) → superreview+
https://hg.mozilla.org/integration/b2g-inbound/rev/80999bc444b5

Setting this to leave-open for the Gaia patch.
Keywords: checkin-needed
Whiteboard: [leave open]
After applying the gaia and the gecko, clicking selectAll in copypaste.html of UI test, the dialog would disappear. 
Still under investigating.
Attached file PR to gaia master
Gaia patch is ready, and waiting for the test result.
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
Attachment #8474249 - Flags: review?(alive)
Attachment #8474249 - Flags: review?(alive) → review+
Thanks,
https://github.com/mozilla-b2g/gaia/commit/86c9735dfd60b21d58207115cdcd5885373969a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.