User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050430 Firefox/1.0+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050430 Firefox/1.0+ See testcase Reproducible: Always Steps to Reproduce:

I don't understand what I'm meant to be seeing/not seeing. Please provide steps to reproduce as well as Expected and Actual results.

The middle line is styled to show text with a red bg-color when you select it. I expect the image to also be highlighted with red instead of the system selection color but that's not happening.

My understanding of ::selection says this is invalid (the selection around the img is not the p::selection, it's the img::selection).

I'm not quite convinced that p::selection wouldn't apply to the image but in any event I am convinced this bug is invalid so I'm marking this as an RFE to add a new CSS property to be able to set the selection color on images since I don't think the background property should be used for image highlighting now that I think about it. I'm thinking maybe -moz-foreground-color that can take an rgba value that acts exactly like the background-color property except it's rendered on top instead of on bottom.

Attached a test case where the img is styled too, and here's some renderings: Firefox: http://i.imgur.com/Iy6N65j.png Chrome: http://i.imgur.com/RPwvf2v.png The first paragraph has #f00 as selection color, which on Firefox gets rendered as that color, but on Chrome, the browser applies an implicit alpha value of around .8, presumably so the image selection isn't a fully-opague block of color. There's another fine detail to Chrome's implementation. When the author supplies a alpha value through rgba(), Chrome applies it, but only if it is not 1, in which case it falls back to the implicit value. Could someone assign this bug to me so I can update its decription and flags?

Updated the bug description to reflect the actual issue here. Also, better samples (the last pngs had transparency set): Chrome: http://i.imgur.com/b5ViQ8B.png Firefox: http://i.imgur.com/GN4QDzB.png

Here's a patch which visually matches the results in other browsers. Basically, I just have nsDisplaySelectionOverlay use an nsTextPaintStyle instance to get the correct background color. This means moving the nsTextPaintStyle class definition into its header file, and making it work with any nsIFrame rather than just nsTextFrames. That in turn means moving nsTextFrame::GetSelectionStatus out into nsIFrame. (Note that I could make it work with nsFrames rather than nsIFrames, but I intend to re-use this stuff for MathML objects in bug 759462, which aren't nsFrames). I also make a couple of tweaks to nsTextPaintStyle's constructor and GetSelectionColors methods so that they're a little more generic for the purposes of images. A try run shows a ton of opt orange, but it all seems like unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8be02611dc287554fa19a3742def7bebcd98df31

(In reply to Thomas Wisniewski from comment #11) > This means moving the nsTextPaintStyle class definition into its header > file, Where possible, it's best to do code-moving changes in a separate (non-functional) patch, so that they can be verified independently, and so that they don't obscure the real functional changes in the "main" patch. Could you do that here? (i.e. please split the nsTextPaintStyle cut-and-paste changes into an initial non-functional "part 1" patch, and then do the rest of your changes in a smaller patch [or patches] that layers on top of that) That'll make review (as well as hg archeology/regression-hunting) easier.

The same goes for the non-functional s/nsTextFrame*/nsIFrame*/ changes inside of the nsTextPaintStyle methods -- those non-functional changes should be performed in an atomic/dedicated patch ("part 2", layering on top of comment 12's "part 1"), to the extent possible. (It looks like the GetSelectionStatus() promotion needs to happen as part of (or before) that patch, too, since nsTextPaintStyle calls GetSelectionStatus() on its frame.)

Sure, I'll try to do that ASAP, thanks for the tips!

Actually, GetSelectionStatus() seems to only have one caller -- this nsTextPaintStyle caller that I mentioned above. And it looks like it doesn't do anything private, so it doesn't really need to be a method on the frame class itself. (though I might be missing something) So, rather than bloating the nsIFrame interface, could we just convert GetSelectionStatus into a static helper function, in its existing location? (i.e. make it take a nsIFrame* argument, instead of being a nsTextFrame/nsIFrame method) If that works, it'd be preferable. (That refactoring should ideally happen in its own patch, too, though it could also happen as part of the helper-patch noted in comment 13 if you prefer.) Thanks!

Comment on attachment 8824196 [details] [diff] [review] 292563-use_selection_bgcolor_for_selected_images.diff Review of attachment 8824196 [details] [diff] [review]: ----------------------------------------------------------------- Some more review feedback: ::: layout/generic/nsFrame.cpp @@ +1746,5 @@ > class nsDisplaySelectionOverlay : public nsDisplayItem { > public: > nsDisplaySelectionOverlay(nsDisplayListBuilder* aBuilder, > + nsFrame* aFrame, int16_t aDisplayType) > + : nsDisplayItem(aBuilder, aFrame), mPaintStyle(aFrame, aDisplayType) { I don't immediately see what this aDisplayType parameter (formerly called aSelectionValue) represents. Could you add a comment here explaining that? (hinting at the sorts of values it takes) (In an ideal world, the values it takes would be an enumerated class -- then it'd be searchable & the compiler would enforce that we're passing in the right thing. But since it's just represented as an integer value right now, there's definitely danger of people passing in the wrong sorts of numbers/constants. :) (or simply having to spin their wheels figuring out which numbers/constants they should be using) So a little documentation would be great.) Also: this might really want to be uint16_t, maybe? (not int16_t) See my note below about the callsite where you're passing in a uint16_t, at least. Or alternately, maybe that callsite should be using an int16_t? @@ +1772,5 @@ > + mPaintStyle.GetSelectionColors(nullptr, &bgColor); > + Color c = Color::FromABGR(bgColor); > + // ensure that the color is at most 80% opacity, so the image still shows > + if (c.a > 0.8) { > + c.a = 0.8; This 0.8-alpha-threshold thing seems like something new. A few thoughts: * The comment here mentions "the image", but I'm not sure this code is image-specific -- it's called by DisplaySelectionOverlay, and there are multiple non-image callers of into DisplaySelectionOverlay[1] -- so that's a bit confusing. So, the language needs a bit of generification. * Perhaps it would be better to perform this *inside of* the nsTextPaintStyle class...? That class does have an EnsureSufficientContrast function already, which seems similar in spirit -- this fixup might want to happen in a method alongside that. (Maybe nsTextPaintStyle::EnsureSelectionColorNotOpaque()?) This might still want to be the only callsite of that function, though -- I'm not sure. [1] https://dxr.mozilla.org/mozilla-central/search?q=DisplaySelectionOverlay @@ +1842,5 @@ > return; > } > > aList->AppendNewToTop(new (aBuilder) > + nsDisplaySelectionOverlay(aBuilder, this, aContentType)); Here this is named "aContentType", but in the implementation of this constructor you're renaming the param to "aDisplayType". Are there any semantic differences between these names? If not, perhaps we should make them consistent one way or another? Also, aContentType is "uint16_t aContentType", but you're passing it in for an arg that has type "int16_t". That seems maybe-broken... ::: layout/generic/nsFrame.h @@ +16,5 @@ > > #include "nsIPresShell.h" > #include "mozilla/ReflowInput.h" > #include "nsHTMLParts.h" > +#include "nsISelectionController.h" This #include doesn't make sense to me here. This file (nsFrame.h) does have a function-declaration with "nsISelectionController**" on it, but we should be able to use a forward-declaration (rather than an #include) to make that OK. (You'd want to add the forward-declare alongside the other ones at around line 106, where we currently have: struct nsBoxLayoutMetrics; struct nsRect; ) Maybe some .cpp file needs this include -- if so, we should add the #include to that .cpp file, rather than here. ::: layout/generic/nsIFrame.h @@ +2931,5 @@ > */ > virtual nsresult GetSelectionController(nsPresContext *aPresContext, nsISelectionController **aSelCon) = 0; > > + int16_t GetSelectionStatus(int16_t* aSelectionFlags) > + { (As noted in comment 15, I'd rather we just left this function in its current location (in nsTextFrame.cpp) and simply convert it into a static helper function instead of a frame method. If we *were* moving this to this nsIFrame.h header, we'd need to add some documentation for consistency, since the nsIFrame interface is important & needs to be well documented. So, all the more reason to avoid moving/touching it. :)) ::: layout/reftests/selection/image-selection-bgcolor-ref.html @@ +1,5 @@ > +<html> > + <head> > + <style type="text/css"> > + body { background-color:yellow; } > + span,img { position:relative; display:block; } As in the testcase: consider using <div> instead of <span>. And I think you really only need to make the container (not the img) position:relative here. The point is for that container to be a containing block for its abspos child, I think. So I think this rule should end up: div { position:relative; } ::: layout/reftests/selection/image-selection-bgcolor.html @@ +1,5 @@ > +<html> > + <head> > + <style type="text/css"> > + body { background-color:yellow; } > + span,img { display:block; } To simplify a bit, can you get rid of this "span,img" CSS rule and just replace your <span>s with <div>s? (Personally I find <span> with display:block to be a bit misleading. And since the <img>s are each inside of their own block-level container, I don't think it matters whether the img elements are themselves display:block.) @@ +3,5 @@ > + <style type="text/css"> > + body { background-color:yellow; } > + span,img { display:block; } > + .img1 img::-moz-selection { background-color:#f00; } > + .img2 img::-moz-selection { background-color:rgba(255,0,0,.5); } Could you express both of these in rgba form, to make the (alpha-channel-only) distinction between them a bit clearer? So the first one should be "rgba(255,0,0,1)" @@ +15,5 @@ > + var range = document.createRange(); > + range.selectNode(imgs[i]); > + selection.addRange(range); > + } > + window.focus(); (You might not need this window.focus() line, if you use "needs-focus" as I suggest in the reftest.list file) @@ +18,5 @@ > + } > + window.focus(); > + } > + function disableReftestWait() { > + document.documentElement.className = ''; This seems unnecessary -- you don't use a "reftest-wait" class in this testcase, so there's nothing to remove here. (This isn't doing anything.) @@ +22,5 @@ > + document.documentElement.className = ''; > + } > + </script> > + </head> > + <body onload="selectAll('img')" onfocus="disableReftestWait()"> Since this testcase doesn't use "reftest-wait", you can probably get rid of this onfocus attribute. ::: layout/reftests/selection/reftest.list @@ +34,5 @@ > == splitText-normalize.html splitText-normalize-ref.html > == modify-range.html modify-range-ref.html > == dom-mutations.html dom-mutations-ref.html > fuzzy-if(OSX==1010,9,1) fuzzy-if(OSX&&skiaContent,6,1) fuzzy-if(skiaContent&&!OSX,1,2138) == trailing-space-1.html trailing-space-1-ref.html > +== image-selection-bgcolor.html image-selection-bgcolor-ref.html I think you need to add "needs-focus" at the beginning of this line.

(Thanks for the patch, btw! I should've started with that :) sorry for jumping straight into nitpicking)

Oh, no problem! I was already expecting a ton of feedback, since I'm new to this part of the codebase to begin with.

(In reply to Daniel Holbert [:dholbert] from comment #16) > @@ +1772,5 @@ > > + mPaintStyle.GetSelectionColors(nullptr, &bgColor); > > + Color c = Color::FromABGR(bgColor); > > + // ensure that the color is at most 80% opacity, so the image still shows > > + if (c.a > 0.8) { > > + c.a = 0.8; > > This 0.8-alpha-threshold thing seems like something new. A few thoughts: > * The comment here mentions "the image", but I'm not sure this code is > image-specific -- it's called by DisplaySelectionOverlay, and there are > multiple non-image callers of into DisplaySelectionOverlay[1] -- so that's a > bit confusing. So, the language needs a bit of generification. > * Perhaps it would be better to perform this *inside of* the > nsTextPaintStyle class...? That class does have an EnsureSufficientContrast > function already, which seems similar in spirit -- this fixup might want to > happen in a method alongside that. (Maybe > nsTextPaintStyle::EnsureSelectionColorNotOpaque()?) This might still want > to be the only callsite of that function, though -- I'm not sure. I'm not sure what the best solution here is either. If we want to only have images make this change (which is fine by me), then maybe I should just pass in the nscolor to use as an optional parameter to DisplaySelectionOverlay (and of course falling back on the existing code if not given)?

(In reply to Thomas Wisniewski from comment #19) > If we want to only have > images make this change (which is fine by me), then maybe I should just pass > in the nscolor to use as an optional parameter to DisplaySelectionOverlay > (and of course falling back on the existing code if not given)? I don't think we want/need to make this an image-specific thing. (If it's bad for the overlay to stomp on images, it'd be bad for it to stomp on anything else, too.) So, to the extent that we draw this overlay on other stuff, we want the same transparency to apply to that other stuff as well. Also: in practice, I'm not sure that we actually draw this overlay at all on non-image stuff right now (perhaps due to a check somewhere up the callstack that I haven't noticed yet). At least, in some brief testing, I was unable to get an overlay to show up for the other sorts elements that appear to call DisplaySelectionOverlay. (I tried <select>, <button>, and <canvas>.) But anyway, if there is some condition under which this overlay shows up on top of other elements, I assume we'd want it to be transparent just like we need it to be for images. (And just like the existing code enforces via "c.a = .5;")

Alright, I've split the patch up and addressed the comments (as far as I can tell). Hopefully I haven't missed anything!

Comment on attachment 8824597 [details] [diff] [review] 292563-1-move_nsTextPaintStyle_to_its_header_file.diff Review of attachment 8824597 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting this up! ::: layout/generic/nsTextFrame.h @@ +16,5 @@ > #include "gfxTextRun.h" > #include "nsDisplayList.h" > #include "JustificationUtils.h" > #include "RubyUtils.h" > +#include "mozilla/LookAndFeel.h" We try to keep #include lists approximately-sorted (with mixed success... :)) So: please move this up higher to be alongside the other "#include mozilla/..." lines. @@ +20,5 @@ > +#include "mozilla/LookAndFeel.h" > + > +using mozilla::SelectionType; > +using mozilla::LookAndFeel; > +using mozilla::TextRangeStyle; Our coding style guide forbids "using" declarations in .h files (because this effectively creates a type alias that leaks out into all .cpp files that #include this header, and that leakiness creates opportunity for sloppiness in those files, e.g. neglecting to add their own required 'using' declarations.) So, please get rid of these statements, and just use these types in their fully-qualified forms in the code that you're moving to this file (so e.g. you'll use "mozilla::SelectionType" instead of "SelectionType" in the pasted code). @@ +840,5 @@ > + * This helper object computes colors used for painting, and also IME > + * underline information. The data is computed lazily and cached as necessary. > + * These live for just the duration of one paint operation. > + */ > +class nsTextPaintStyle { Now that this is a publicly-exposed type, the final sentence in this documentation is a bit more of a hope than a promise. (Previously, this type's uses were limited to nsTextFrame.cpp, which could pretty easily be sanity-checked by hand -- but now that it's exposed, any gecko code can allocate one of these and hold onto it for as long as it wants, in violation of this documentation.) So: to strengthen this documentation with a bit more of a guarantee -- could you tag this class as MOZ_STACK_CLASS, to ensure that it's only ever allocated on the stack? That will help ensure that these really are temporary objects. See https://dxr.mozilla.org/mozilla-central/search?q=MOZ_STACK_CLASS for sample usage and https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/mfbt/Attributes.h#370-376 for documentation, if you're curious. You'll also need to add... #include "mozilla/Attributes.h" ...to provide the definition for this annotation. And to be sure this compiles (i.e. that no current usages violate the requirements for MOZ_STACK_CLASS), you'll need to do a try build on one of the "static analysis" platforms (or build locally with clang as your compiler and --enable-clang-plugin in your mozconfig). This change could happen as part of this patch (assuming it Just Works) or as part of a helper patch -- whichever you prefer.

Comment on attachment 8824598 [details] [diff] [review] 292563-2-change_nsTextPaintStyle_to_work_with_nsIFrames.diff Review of attachment 8824598 [details] [diff] [review]: ----------------------------------------------------------------- r=me on this part, with one whitespace nit: ::: layout/generic/nsTextFrame.cpp @@ +3780,5 @@ > +{ > + // get the selection controller > + nsCOMPtr<nsISelectionController> selectionController; > + nsresult rv = aFrame->GetSelectionController(aFrame->PresContext(), > + getter_AddRefs(selectionController)); Indentation is off here (because of the "aFrame->" that you're adding on the first line of this function call). To fix the indentation without going over 80 characters per line, please wrap after the "=" (and then make sure the next 2 lines align nicely), like so: nsresult rv = aFrame->GetSelectionController(aFrame->PresContext(), getter_AddRefs(selectionController));

(In reply to Daniel Holbert [:dholbert] from comment #20) > Also: in practice, I'm not sure that we actually draw this overlay at all on > non-image stuff right now (perhaps due to a check somewhere up the callstack > that I haven't noticed yet). At least, in some brief testing, I was unable > to get an overlay to show up for the other sorts elements that appear to > call DisplaySelectionOverlay. (I tried <select>, <button>, and <canvas>.) I figured out what the problem was -- by default, only a few things are selectable (controlled by the "-moz-user-select" property). Testcases need to set "-moz-user-select:all" on some root node in order for its non-image/text children to be selectable (and to get an overlay drawn). Alternately/more-thoroughly, the "contenteditable" attribute makes more stuff selectable as well.

So, there are a couple of cosmetic differences that I'm noticing after applying this patch stack. (1) the default text-selection color becomes noticeably lighter (and selected text is harder-to-read as a result), as compared to an unpatched build. I think this is due to the 0.8 clamping happening more frequently than we'd actually like it to. This is visible in e.g. the URL bar, as well as on simple pages like: data:text/plain,select this text (2) Similarly, the grayed out text-selection color in an *unfocused* window is noticeably lighter in a patched build vs. an unpatched build. We need to be sure we're not regressing these and changing the default UI & colors by accident here.

To illustrate my previous comment, here are some screenshots to show the patch's effects on the default text-selection experience (effects that we'd like to avoid).

Thanks for the input! I'll try to get back to these patches soon. >please move this up higher to be alongside the other "#include mozilla/..." lines. Will do. The "approximately sorted" thing is something I'll have to get a feel for, it seems :) >"using" declarations in .h files >MOZ_STACK_CLASS >clang/--enable-clang-plugin >-moz-user-select Good to know about these things, thanks. I'll definitely fire up a separate clang build so I can test other patches more quickly. >So, there are a couple of cosmetic differences that I'm noticing after applying this patch stack. That would almost certainly be due to me moving the 0.8-clamping into the general selection code. I guess that means I'll have to limit that change to just nsDisplaySelectionOverlay after all (and perhaps only to images specifically).

I don't think it should be image-specific -- it's just as applicable for the other non-text elements in my latest testcases here, too. (E.g. <canvas>, <button>) But yeah, it should probably only happen from nsDisplaySelectionOverlay. What I had in mind was: - nsTextPaintStyle should grow a new method called EnsureSelectionColorsTransparent() or something like that, which triggers the "lazily compute selection colors" codepath, and then clamps these selection colors. - This new method should *only* be called from nsDisplaySelectionOverlay (before it asks for the selection colors). This lets nsDisplaySelectionOverlay stay relatively "dumb", and it keeps the color-adjustment logic all in one place. How does that sound?

>How does that sound? Sounds fine to me; it's what I had in mind as well.

Alright, I've finally gone ahead and updated the patches with review comments addressed. It's been a couple of months, so I'm guessing you'll want to check them out all over again. Try seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab5e1bc8f812a9f3a71469d974a41857f46bba3e

Note that I've updated my reftest in this patch to take your above tests in comments 29 and 30 into account as well.

Hmm. There was an odd reftest failure I can't figure out on OSX, but I'm not sure if fuzzing is the way to go. See https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://archive.mozilla.org/pub/firefox/try-builds/wisniewskit@gmail.com-ab5e1bc8f812a9f3a71469d974a41857f46bba3e/try-macosx64/try_yosemite_r7_test-reftest-e10s-bm107-tests1-macosx-build1590.txt.gz&only_show_unexpected=1 What I'm doing for the reference image is covering those elements with a translucent div that has same the same dimensions as they do (using getBoundingClient). I'm not sure why it only fails for the two top groups of elements, but not the bottom two.

I just fixed this in bug 1449010.

Thomas: I'm very sorry for leaving these review requests for so long -- I definitely stretched the "no rush, when you have time to get to it" allowance past the breaking point here. I just noticed that emilio noted that he's fixed this, though, so I'll go ahead and cancel the review requests. In any case, I owe you a beverage/pastry/equivalent of your choice, plus swifter reviews on any future patches!