Update selection to handle new skb functionality

RESOLVED FIXED in Firefox 23

Status

Firefox for Metro
Input
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #857823 +++
(Assignee)

Comment 1

5 years ago
Created attachment 733078 [details] [diff] [review]
part 1 - cleanup
Assignee: nobody → jmathies
(Assignee)

Comment 2

5 years ago
Created attachment 733080 [details] [diff] [review]
part 2 - handle DeckOffset events
(Assignee)

Comment 3

5 years ago
Created attachment 733356 [details] [diff] [review]
part 1 - cleanup
Attachment #733080 - Attachment is obsolete: true
Attachment #733078 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 733359 [details] [diff] [review]
part 2 - handle DeckOffset events
(Assignee)

Comment 5

5 years ago
Created attachment 733361 [details] [diff] [review]
part 3 - merge closeEditSession calls
(Assignee)

Comment 6

5 years ago
Created attachment 733363 [details] [diff] [review]
part 4 - remove pointer-events: none from selection overlay
(Assignee)

Comment 7

5 years ago
Created attachment 733365 [details] [diff] [review]
part 4 - remove pointer-events: none from selection overlay
Attachment #733363 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Comment on attachment 733356 [details] [diff] [review]
part 1 - cleanup

Please forgive me. :)

This is just some format cleanup of SelectionHelperUI.
Attachment #733356 - Flags: review?(mbrubeck)
(Assignee)

Comment 9

5 years ago
Comment on attachment 733359 [details] [diff] [review]
part 2 - handle DeckOffset events

reposition form elements when they are obscured by the soft keyboard.
Attachment #733359 - Flags: review?(mbrubeck)
(Assignee)

Comment 10

5 years ago
Comment on attachment 733361 [details] [diff] [review]
part 3 - merge closeEditSession calls

minor housekeeping, coalesce two api calls in SelectionHelperUi into one.
Attachment #733361 - Flags: review?(mbrubeck)
(Assignee)

Comment 11

5 years ago
Comment on attachment 733365 [details] [diff] [review]
part 4 - remove pointer-events: none from selection overlay

The icing on the cake - remove the selection overlay event trap.

There is a rollup in the parent bug of everything I've been working on. If you'd like to take it for a spin I can fire up a try build.
Attachment #733365 - Flags: review?(mbrubeck)
Attachment #733356 - Flags: review?(mbrubeck) → review+
Comment on attachment 733359 [details] [diff] [review]
part 2 - handle DeckOffset events

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

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +817,5 @@
> +  },
> +
> +  _onResize: function _onResize() {
> +    this._sendAsyncMessage("Browser:SelectionUpdate", {});
> +  },

Did part of this hunk get duplicated from the previous patch?

@@ +846,5 @@
> +
> +  _onDebugRectRequest: function _onDebugRectRequest(aMsg) {
> +    this.overlay.addDebugRect(aMsg.left, aMsg.top, aMsg.right, aMsg.bottom,
> +                              aMsg.color, aMsg.fill, aMsg.id);
> +  },

More duplicate code.
Attachment #733359 - Flags: review?(mbrubeck) → review+
Comment on attachment 733361 [details] [diff] [review]
part 3 - merge closeEditSession calls

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

::: browser/metro/base/content/ContextCommands.js
@@ +65,5 @@
>        // content
>        if (ContextMenuUI.popupState.string) {
>          this.sendCommand("copy");
>  
> +        SelectionHelperUI.closeEditSession((false, true));

Extra ((parens)) snuck in here.

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +385,2 @@
>     */
> +  closeEditSession: function closeEditSession(aClearFocus, aClearSelection) {

Do we ever pass aClearFocus=true anymore? If not, we could remove it.

@@ +940,5 @@
>  
>        case "ZoomChanged":
>        case "URLChanged":
>        case "MozPrecisePointer":
> +        this.closeEditSession((false, true));

((and here))
Attachment #733361 - Flags: review?(mbrubeck) → review+
Comment on attachment 733365 [details] [diff] [review]
part 4 - remove pointer-events: none from selection overlay

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

::: browser/metro/theme/browser.css
@@ +270,5 @@
>  }
>  
> +.selection-overlay {
> +  pointer-events: none;  
> +}

Would it be reasonable to get rid of the overlay binding completely and just have the monocles directly in browser.xul?  Or do we still get some use out of having an overlay?
Attachment #733365 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 15

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Comment on attachment 733359 [details] [diff] [review]
> part 2 - handle DeckOffset events
> 
> Review of attachment 733359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/helperui/SelectionHelperUI.js
> @@ +817,5 @@
> > +  },
> > +
> > +  _onResize: function _onResize() {
> > +    this._sendAsyncMessage("Browser:SelectionUpdate", {});
> > +  },
> 
> Did part of this hunk get duplicated from the previous patch?
> 
> @@ +846,5 @@
> > +
> > +  _onDebugRectRequest: function _onDebugRectRequest(aMsg) {
> > +    this.overlay.addDebugRect(aMsg.left, aMsg.top, aMsg.right, aMsg.bottom,
> > +                              aMsg.color, aMsg.fill, aMsg.id);
> > +  },
> 
> More duplicate code.

Bad merge, updated.
(Assignee)

Comment 16

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #13)
> Comment on attachment 733361 [details] [diff] [review]
> part 3 - merge closeEditSession calls
> 
> Review of attachment 733361 [details] [diff] [review]:
> -----------------------------------------------------------------

Updated per comments.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.