Last Comment Bug 667062 - Content text selection highlight should match Gingerbread theming style
: Content text selection highlight should match Gingerbread theming style
Status: VERIFIED FIXED
[testday-2011-06-24]
: polish, verified-aurora
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: -- enhancement (vote)
: Firefox 7
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on:
Blocks: 653134 661388
  Show dependency treegraph
 
Reported: 2011-06-24 14:34 PDT by Aaron Train [:aaronmt]
Modified: 2011-08-05 06:49 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Stock browser content text selection - white background (45.31 KB, image/png)
2011-06-24 14:34 PDT, Aaron Train [:aaronmt]
no flags Details
Stock browser content text selection - black background (87.41 KB, image/png)
2011-06-24 14:35 PDT, Aaron Train [:aaronmt]
no flags Details
patch plus more (5.47 KB, patch)
2011-07-12 14:35 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-06-24 14:34:40 PDT
Created attachment 541802 [details]
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] 2011-06-24 14:35:05 PDT
Created attachment 541804 [details]
Stock browser content text selection - black background
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-12 14:35:52 PDT
Created attachment 545487 [details] [diff] [review]
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) 2011-07-13 09:31:31 PDT
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?) 2011-07-13 11:11:03 PDT
(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?) 2011-07-13 12:04:30 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/225932c41c03
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-14 09:31:10 PDT
http://hg.mozilla.org/mozilla-central/rev/225932c41c03
Comment 7 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-07-15 13:25:27 PDT
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?) 2011-08-04 12:32:05 PDT
Comment on attachment 545487 [details] [diff] [review]
patch plus more

mobile only and it helps stabilize the text selection feature
Comment 9 christian 2011-08-04 14:37:13 PDT
Comment on attachment 545487 [details] [diff] [review]
patch plus more

Approved for mozilla-aurora
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-04 20:14:50 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c6be7aeb4ef9
Comment 11 Aaron Train [:aaronmt] 2011-08-05 06:49:09 PDT
Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2

Note You need to log in before you can comment on or make changes to this bug.