667062 - Content text selection highlight should match Gingerbread theming style

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Aaron Train [:aaronmt]]

Attachment: Stock browser content text selection - white background

Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110624 Firefox/7.0a1 Fennec/7.0a1

Currently content text selection is selected with a black background alongside grey text. Text that is highlighted becomes difficult to read. This should match the gingerbread style (stock browser)

For white backgrounds stock browser uses
Highlight selection: #FED27E

Black backgrounds stock browser uses
Highlight selection: #805400

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Stock browser content text selection - black background

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch plus more

This patch adds the CSS change and a few small fixes:
* -moz-selection in web content so we use the orange color in web content.
* Adds protection for the error in bug 667097. We need to avoid touching the selection object in the "end" message, so we cache the selected text during the "start" and "move" messages.
* Adds basic protection to avoid messing up the focus and selection if you move the handles over a textbox. This is fixed by the platform patch that allows us to not use fake clicks (bug 669816).

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 545487 [details] [diff] [review]
patch plus more

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

Looks fine and works fine here.

::: mobile/chrome/content/content.js
@@ +1407,5 @@
>  
>        case "Browser:SelectionMove":
> +        // Hack to avoid setting focus in a textbox [Bugs 654352 & 667243]
> +        let elemUnder = elementFromPoint(json.x - scrollOffset.x, json.y - scrollOffset.y);
> +        if (elemUnder && elemUnder instanceof Ci.nsIDOMHTMLInputElement || elemUnder instanceof Ci.nsIDOMHTMLTextAreaElement)

I half wonder if we need to combine this into a Utils function since we seem to check this sort of stuff other places:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#849

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)

> >        case "Browser:SelectionMove":
> > +        // Hack to avoid setting focus in a textbox [Bugs 654352 & 667243]
> > +        let elemUnder = elementFromPoint(json.x - scrollOffset.x, json.y - scrollOffset.y);
> > +        if (elemUnder && elemUnder instanceof Ci.nsIDOMHTMLInputElement || elemUnder instanceof Ci.nsIDOMHTMLTextAreaElement)
> 
> I half wonder if we need to combine this into a Utils function since we seem
> to check this sort of stuff other places:
> 
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#849

If it was a long term fix, I would support moving to a util method, but we want to remove this if possible.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/225932c41c03

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/225932c41c03

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv8.0a1) Gecko/20110715 Firefox/8.0a1 Fennec/8.0a1
Device: HTC Flyer
OS: Android OS 2.3

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 545487 [details] [diff] [review]
patch plus more

mobile only and it helps stabilize the text selection feature

Comment 9

[LegNeato]

Comment on attachment 545487 [details] [diff] [review]
patch plus more

Approved for mozilla-aurora

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/c6be7aeb4ef9

Comment 11

[Aaron Train [:aaronmt]]

Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2