167068 - [CSS3] implement ::selection pseudo-element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Implement the CSS 3 ::selection pseudo-element.
The spec is available at http://www.w3.org/TR/css3-selectors/#UIfragments
CSS properties that apply to ::selection pseudo-elements are : color, cursor,
background, outline.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v0.9

Mike, this patch does everything needed to deal with the background-color of
the
selection. The other properties we have to deal with in the context of
-moz-selection are

  color
  cursor
  outline
  background-image
  background-attachment
  background-position
  background-repeat

to be honest, I don't like the 4 last ones in the context of the selection.
Attaching later a test case for this patch.

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case for :-moz-selection

Comment 3

[Eli Friedman]

Shouldn't that be "::-moz-selection"? (Psudo-element, not psudo-class)

Comment 4

[Hixie (not reading bugmail)]

Minor nit:

+class CSSSelectionRule : public CSSFirstLineRule {

That seems wrong. Shouldn't they both inherit from a common ancestor?


Otherwise, interesting stuff!

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Oh.  Ugh.  I should probably finish the patch in bug 103189 at some point.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Actually, though, you probably don't need that special selection rule, since
you're not creating a frame with this style context -- you're actually just
reimplementing the allowed properties in a one-off sort of way.

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> Minor nit:
>
> +class CSSSelectionRule : public CSSFirstLineRule {
>
> That seems wrong. Shouldn't they both inherit from a common ancestor?

Right, I did that too fast. But please note that CSSFirstLetterrRule also
inherits from that class. I just did the same mistake.

> Otherwise, interesting stuff!

What's interesting is not my code. It just reuses the superb power of Mike
Judge's code and design. Adding ::selection to his code is a child's play
and I should have done that looooong ago.

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> Shouldn't that be "::-moz-selection"? (Psudo-element, not psudo-class)

Absolutely. The patch is ready but not checked in yet. This is covered by another
bug and I'll make it land asap since we absolutely need it to be able to exit
Selectors CR.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> Oh.  Ugh.  I should probably finish the patch in bug 103189 at some point.

David, do you want to do that first or can I move forward w/o waiting for your
check-in ?

(yes, I also saw that CSSFirstLineRule::MapRuleInfoInto was involved into
 bug 103189)

OOOOOPSSSS, marked RESO/FIXED by error, sorry for that, my mouse is going to
spend the next ten days in hell as a punishment.

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

sorry for spam

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I actually don't think it matters, since I don't think you need the
CSSSelectionRule at all to fix this bug.  (See comment 6.)

Comment 12

[rbs]

+    nsIStyleContext* sc = nsnull;

Seems to leak.

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: more complete and better test case

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: much better case again, with correct image URL

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0

I think that this patch is ok for reviews. David, Mike, Boris, care to r/sr
please ? You can test the patch with the attached test case.

Comment 16

[mjudge]

just because I have been through this before I am going to be very picky here.  

+    nsIStyleContext *sc = nsnull;

should probably be an nsCOMPtr. I see that you may want to release it sooner.
(for all I know you copied this from my own code) if you want when you call

+      NS_RELEASE(sc)

just instead set sc=0; to force a free of the nsCOMPtr at that time

*Note: thanks for cleaning up these tabs i had in here somehow.

Looks like tab wierdness near bottom of patch

**
 	      if (newDimensions.width)
 	      {
-		      if (iter.CurrentBackGroundColor(currentBKColor))
+		      if (iter.CurrentBackGroundColor(currentBKColor,
&isCurrentBKColorTransparent))
 		      {//DRAW RECT HERE!!!
-		      aRenderingContext.SetColor(currentBKColor);
-		      aRenderingContext.FillRect(currentX, dy, newDimensions.width,
mRect.height);
-						      currentFGColor = EnsureDifferentColors(currentFGColor, currentBKColor);
+		        if (!isCurrentBKColorTransparent) {
+                  aRenderingContext.SetColor(currentBKColor);
+                  aRenderingContext.FillRect(currentX, dy, newDimensions.width,
mRect.height);
+                }
+                currentFGColor = EnsureDifferentColors(currentFGColor,
currentBKColor);
 		      }
 	      }
**


One last thing which I am sure you are not looking forward to..  with 14835
which is in the trunk right now I use the seleciton color to alpha blend the
image that is selected.  Does this newly implemented style rule affect this?  We
may have to talk about how to best solve this one.

Comment 17

[mjudge]

put down an r for me after you address the things i said.  either change the
comptr, fix the indents or not I leave it to you. r=mjudge

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.1

this patch in answer to mjudge's comments
also fixes indentation and coding style in the whole file since I had a lot
of difficulty to read some portions of it

Kin, for your super-review, please prefer the "clean" part in patch v1.0 above

Comment 19

[kinmoz]

Comment on attachment 99519 [details] [diff] [review]
patch v1.1

Yowza, this diff went from ~300 lines to ~2100 lines ... I think next time it
would be a lot easier to do the formatting sweep in some other diff so it
doesn't hide your changes, this would also make it easier on reviewers too.


==== This needs the args to line up like the rest of your changes:


+  NS_IMETHOD CheckVisibility(nsIPresContext* aContext,
+			  PRInt32 aStartIndex,
+			  PRInt32 aEndIndex,
+			  PRBool aRecurse,
+			  PRBool *aFinished,
+			  PRBool *_retval);


==== This doesn't line up properly:


   NS_IMETHOD  GetChildFrameContainingOffset(PRInt32	 inContentOffset,
-				 PRBool 		 inHint,
-				 PRInt32*		 outFrameContentOffset,
-				 nsIFrame*		 *outChildFrame);
+					     PRBool		     inHint,
+					     PRInt32*		    
outFrameContentOffset,
+					     nsIFrame*		    
*outChildFrame);



==== Shouldn't we check for |aContent == nsnull| before dereferencing
|aContent|? See later comments.


+    nsCOMPtr<nsIStyleContext> sc;
+    nsCOMPtr<nsIContent> parentContent;
+    aContent->GetParent(*getter_AddRefs(parentContent));
+    aPresContext->ProbePseudoStyleContextFor(parentContent,


==== Do we have to check for null before dereferencing |bg| or |color|?


+      const nsStyleBackground* bg = (const 

nsStyleBackground*)sc->GetStyleData(eStyleStruct_Background);


+      const nsStyleColor* color =(const nsStyleColor*)
sc->GetStyleData(eStyleStruct_Color);


==== This doesn't look like it lines up right:


-			  aTextStyle.mNormalFont->GetUnderline(offset, size);
+	 aTextStyle.mNormalFont->GetUnderline(offset, size);
	       aRenderingContext.SetColor(IME_CONVERTED_COLOR);
	       aRenderingContext.FillRect(aX + startOffset+size, aY + baseline
-
offset, 

textWidth-2*size, size);
				 }break;

==== In 4 different places, you moved the |content| declaration outside an |if|
block, which was responsible for retrieving it's value, and then later you pass
|content| into |DrawSelectionIterator iter()| ... if |content| is ever null, we
will crash because the |DrawSelectionIterator| constructor doesn't check the
passed in content node ptr for null before dereferencing it ... perhaps a check
should be added to the constructor:


-    if (NS_SUCCEEDED(rv) && frameSelection){
-      nsCOMPtr<nsIContent> content;
+    nsCOMPtr<nsIContent> content;
+    if (NS_SUCCEEDED(rv) && frameSelection) {

...

-    DrawSelectionIterator iter(details,text,(PRUint32)textLength, ts, 

nsISelectionController::SELECTION_NORMAL, aPresContext);
+    DrawSelectionIterator iter(content, details,text,(PRUint32)textLength, ts, 

nsISelectionController::SELECTION_NORMAL, aPresContext, mStyleContext);


==== Line the comment up and move the comment after the ';' to under the
comment
it belongs.


-	   //increment twips X start but remember to get ready for next draw by
reducing 

current x by letter spacing amount
-	      currentX+=newDimensions.width;// + aTextStyle.mLetterSpacing;
+	     //increment twips X start but remember to get ready for next draw
by reducing 

current x by letter spacing amount
+	   currentX+=newDimensions.width;// + aTextStyle.mLetterSpacing;


==== Fix the indentation:


+	       if (!isCurrentBKColorTransparent) {
+		 aRenderingContext.SetColor(currentBKColor);
+		 aRenderingContext.FillRect(currentX, dy, newWidth,
mRect.height);
+	       }
+	 currentFGColor = EnsureDifferentColors(currentFGColor,
currentBKColor);


==== Why aren't we setting the color anymore here?


	   if (isPaginated && !iter.IsBeforeOrAfter()) {
-	     

aRenderingContext.SetColor(nsCSSRendering::TransformColor(aTextStyle.mColor->mC
olor,canDarkenC

olor));
	     aRenderingContext.DrawString(currenttext, currentlength, currentX,
dy + mAscent);


==== This still doesn't line up correctly:


 TryNextFrame:	      
-		  GetNextInFlow(&frameUsed);
+      GetNextInFlow(&frameUsed);
	 start = 0;
       }

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> Yowza, this diff went from ~300 lines to ~2100 lines ... I think next time it
> would be a lot easier to do the formatting sweep in some other diff so it
> doesn't hide your changes, this would also make it easier on reviewers too.

That's why I said above 'Kin, for your super-review, please prefer the "clean"
   part in patch v1.0 above'...

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.1, modifs only, no indent cleanup

Kin, this one is more readable and answers to your comments.
About the result of sc->GetStyle(), no need to null check it.

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 99958 [details] [diff] [review]
patch v1.1, modifs only, no indent cleanup

carries forward r=mjudge

Comment 23

[kinmoz]

Comment on attachment 99958 [details] [diff] [review]
patch v1.1, modifs only, no indent cleanup

sr=[e-mail]

Just fix the indentation here:


	      if (newDimensions.width)
	      {
-		      if (iter.CurrentBackGroundColor(currentBKColor))
+		      if (iter.CurrentBackGroundColor(currentBKColor,
&isCurrentBKColorTransparent))
		      {//DRAW RECT HERE!!!
-		      aRenderingContext.SetColor(currentBKColor);
-		      aRenderingContext.FillRect(currentX, dy,
newDimensions.width, mRect.height);
-						      currentFGColor =
EnsureDifferentColors(currentFGColor, currentBKColor);
+		 if (!isCurrentBKColorTransparent) {
+		   aRenderingContext.SetColor(currentBKColor);
+		   aRenderingContext.FillRect(currentX, dy,
newDimensions.width, mRect.height);
+		 }
+		 currentFGColor = EnsureDifferentColors(currentFGColor,
currentBKColor);
		      }
	      }

Comment 24

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in trunk

Comment 25

[Kai Lahmann (is there, where MNG is)]

why is it -moz ?

Comment 26

[Hixie (not reading bugmail)]

see bug 176170

Comment 27

[Erik Arvidsson]

This does not work with xul:textbox and html:textarea and other form elements.

Test case coming up

Comment 28

[Erik Arvidsson]

Attachment: Test case with html:textarea and html:input

Comment 29

[Hixie (not reading bugmail)]

Erik: Please file a separate bug (and mark it as blocking bug 176170).

This bug was about the initial implementation, which is done. Keeping each issue
separate results in patches being much easier to track.

Cheers.