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 | Splinter Review

Description User image 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 User image Aaron Train [:aaronmt] 2011-06-24 14:35:05 PDT
Created attachment 541804 [details]
Stock browser content text selection - black background
Comment 2 User image 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 User image 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 User image 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 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-07-13 12:04:30 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/225932c41c03
Comment 6 User image :Ehsan Akhgari (in Taipei, laggy response time) 2011-07-14 09:31:10 PDT
http://hg.mozilla.org/mozilla-central/rev/225932c41c03
Comment 7 User image 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 User image 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 User image christian 2011-08-04 14:37:13 PDT
Comment on attachment 545487 [details] [diff] [review]
patch plus more

Approved for mozilla-aurora
Comment 10 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-08-04 20:14:50 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c6be7aeb4ef9
Comment 11 User image 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.