Bug 21680 – queryCommandValue("BackColor") returns rgb(0,0,0) for elements with transparent background

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

queryCommandValue for "BackColor" always returns rgb(0,0,0) in Safari, no matter what the background color is. This is not only not correct, it's also not in the correct format. The other browsers (Firefox, IE) returns the background color in hex form (ie #FFFFFF).

[Ryosuke Niwa]

Created attachment 35008 [details]
Demonstrates the bug

Not always.  Whenever background color is specified in an ancestor of a node, it returns rgba(0,0,0,0).

[Ryosuke Niwa]

This bug is caused by PassRefPtr<CSSComputedStyleDeclaration> Frame::selectionComputedStyle(Node*& nodeToRemove) const.

I added styleElement->showTreeForThis(); at the end and probed my demo.

BODY	0x1ac54250
	#text	0x1acf72f0 "\n"
	DIV	0x1acf9360 STYLE=background: green;
		SPAN	0x1acf7b10
			#text	0x1c61a080 "hello world"
	#text	0x1ac53740 "\n\n"
*	P	0x1acf6d60
		#text	0x1acf7150 "BackgroundColor: "
		SPAN	0x1acf6b90
	#text	0x1ac28600 "\n\n"

This isn't right.  P is not even selected.

[Ryosuke Niwa]

RefPtr<Range> range(selection()->toNormalizedRange());
After this, range->startPosition() is at P.

[Ryosuke Niwa]

Created attachment 35075 [details]
demo

[Ryosuke Niwa]

My demo had some bug to cause position / range not to show up properly.  Now the issue is that computed style returns rgba(0,0,0,0) unless the background color is explicitly specified for that node.  We might need to traverse the tree upwards from the selected node to find the background color.  But what happens when the node is absolutely positioned?

[Ryosuke Niwa]

Created attachment 51244 [details]
more tests

More test cases:
1. The effective background color is of a parent element
2. The background color is explicitly specified
3. An element is placed with position:absolute and inherits the document's default background.
4. An element is placed with position:absolute on top of another element with an explicit background color.

Sadly, all major browsers give different results, and MSIE gives the most reasonable result (passes 1 & 2 and returns white for 3 & 4).  But I'm not sure of the expected results for the last two tests (3 & 4).  From implementors' perspective, they are quite unreasonable because we need to extract the color value from the rendered image.  We need CSS / rendering specialists on deciding what is the most appropriate behavior here.

Also, we probably need a test with various alpha values.  Consider a situation where an element has alpha=50%.  Then the background color will be mixed with that of the parent element's background color.  Should we return the effective value? or should we just return the value without alpha?

[Ojan Vafai]

Lets keep this bug focused on the following case:
<div style="background-color:blue">foo<span id="select-this">bar</span></div>

Dealing with absolute positioning or transparency should be separate, lower priority bugs.

[Ryosuke Niwa]

(In reply to comment #7)
> Lets keep this bug focused on the following case:
> <div style="background-color:blue">foo<span id="select-this">bar</span></div>
> 
> Dealing with absolute positioning or transparency should be separate, lower
> priority bugs.

Sure.  We can file a separate bug for those cases.  But we probably still need to consider different values of alpha since the only way to detect this case to look at the computed style and see if the alpha value of the background color is 0.  What if alpha=0.5 or other values?  We should return rgba(0,0,0, 0.5)?

[Ojan Vafai]

It's not at all clear to me what the correct treatment of transparency and absolute positioning is with respect to background-color. It is clear that it's much lower priority than this bug though. I'm not really convinced that we have to deal with transparency and absolute positioning. If we find ourselves with a site that needs it though, then we can discuss it then with a real use-case in front of us.

[Ryosuke Niwa]

Created attachment 61003 [details]
tests with multiple background colors

Added tests with multiple span of different background colors.

[Ryosuke Niwa]

I ran the latest tests on Firefox and Internet Explorer to see their behaviors.

Internet Explorer returns rgb(0, 0, 0) for mixed background colors.  e.g. <div style='background: green;' id=test><span style='background-color: yellow'>hello</span><span style='background-color: blue'> world</span></div> gives rgb(0,0,0) when #test is selected.

In other cases, IE retrieves the effective background color. For <div style='background: green;' id=test><span style='background-color: yellow'>hello</span><span style='background-color: yellow'> world</span></div>, IE returns yellow (in RGB form).

Firefox does something somewhat simpler.  It retrieves the background color of the node selected, and traverses ancestor nodes while the background color of the selected node is transparent.  So <div style='background: green;' id=test><span style='background-color: yellow'>hello</span><span style='background-color: blue'> world</span></div> AND <div style='background: green;' id=test><span style='background-color: yellow'>hello</span><span style='background-color: yellow'> world</span></div> BOTH gives green.

It seems like IE is doing a good job in terms of what user would expect but I'm not sure if we should return black in the case of mixed background color. Ojan & Julie, any thoughts on this?

Also, during the debugging, I found that CSSComputedStyleDeclaration always returns RGBA (RGB+Alpha value) for CSSPropertyBackgroundColor regardless of whether it's transparent or not.  So in the case of a node with background-color: transparent, we still get rgba(0,0,0,0).  It seems like we need to special case this value or modify the computed style to return CSSValueTransparent.  Does anyone know with whom I should talk about this issue?

[Ryosuke Niwa]

Created attachment 61087 [details]
merged valueStyle in EditorCommand.cpp and Frame::selectionStartStylePropertyValue and added a special case to treat background color

This patch merges valueStyle in EditorCommand.cpp and Frame::selectionStartStylePropertyValue because selectionStartStylePropertyValue was only used in valueStyle.  I added a special case for background color where I check whether the style has transparent background or the current selection is a range.  In either cases, new valueStyle will traverse the tree upwards until it gets a computed style with non-transparent background.  I could improve the change log so give me some feedback please.

[WebKit Review Bot]

Attachment 61087 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/EditorCommand.cpp:240:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/editing/EditorCommand.cpp:248:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/editing/EditorCommand.cpp:247:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/editing/EditorCommand.cpp:257:  Declaration has space between type name and * in Node *nodeToRemove  [whitespace/declaration] [3]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 61094 [details]
fixed style: merged valueStyle in EditorCommand.cpp and Frame::selectionStartStylePropertyValue and added a special case to treat background color

[Darin Adler]

Comment on attachment 61094 [details]
fixed style: merged valueStyle in EditorCommand.cpp and Frame::selectionStartStylePropertyValue and added a special case to treat background color

The special case for background color, climbing the tree and finding backgrounds on other elements, seems strange to me and does not seem like the right way to go. Do other browsers have behavior like this?

Instead of "value->getRGBA32Value() == makeRGBA(0, 0, 0, 0)" you should just check for an alpha of 0.

Getting selectionStartStylePropertyValue out of the Frame class is a good idea. Putting all this logic into EditorCommand.cpp is not so good. The point of that file is to be a dispatch point for the various editing commands. Any command that has a nontrivial algorithm should be contained in functions in other files. This algorithm in particular is complex and I'm not sure that having it here is good.

[Ryosuke Niwa]

Thanks for the feedback!

(In reply to comment #15)
> (From update of attachment 61094 [details])
> The special case for background color, climbing the tree and finding backgrounds on other elements, seems strange to me and does not seem like the right way to go. Do other browsers have behavior like this?

As far as I checked, yes.  See the comment #11.  We could look at the actual color rendered on the screen as well but that seems bizarre to me.


> Instead of "value->getRGBA32Value() == makeRGBA(0, 0, 0, 0)" you should just check for an alpha of 0.

I was thinking that as well.  I'll add more tests for the cases where alpha=0 but background color is not transparent.


> Getting selectionStartStylePropertyValue out of the Frame class is a good idea. Putting all this logic into EditorCommand.cpp is not so good. The point of that file is to be a dispatch point for the various editing commands. Any command that has a nontrivial algorithm should be contained in functions in other files. This algorithm in particular is complex and I'm not sure that having it here is good.

Ok, I had a hunch for it.  Editor class seems to be a good place to put especially around triStateOfStyleInComputedStyle, selectionHasStyle, etc...

[Ojan Vafai]

(In reply to comment #11)
> It seems like IE is doing a good job in terms of what user would expect but I'm not sure if we should return black in the case of mixed background color. Ojan & Julie, any thoughts on this?

IE's behavior is clearly best in my opinion. While I agree that it's not ideal to return black in the indeterminate case, it's better than what WebKit/Gecko do. Importantly, document.queryCommandIndeterm('BackColor') in indeterminate cases returns true in IE. So web pages are able to distinguish a black background color from an indeterminate one. It returns false in WebKit/Gecko, which is clearly wrong, although that's a separate bug entirely.

> Also, during the debugging, I found that CSSComputedStyleDeclaration always returns RGBA (RGB+Alpha value) for CSSPropertyBackgroundColor regardless of whether it's transparent or not.  So in the case of a node with background-color: transparent, we still get rgba(0,0,0,0).  It seems like we need to special case this value or modify the computed style to return CSSValueTransparent.  Does anyone know with whom I should talk about this issue?

I agree that this seems wrong. Again, it's worth checking what IE/Firefox do in this case.

[Eric Seidel (no email)]

We need to be careful in these changes to not leak visited link information. :)

[Ryosuke Niwa]

(In reply to comment #17)
> (In reply to comment #11)
> > It seems like IE is doing a good job in terms of what user would expect but I'm not sure if we should return black in the case of mixed background color. Ojan & Julie, any thoughts on this?
> 
> IE's behavior is clearly best in my opinion. While I agree that it's not ideal to return black in the indeterminate case, it's better than what WebKit/Gecko do. Importantly, document.queryCommandIndeterm('BackColor') in indeterminate cases returns true in IE. So web pages are able to distinguish a black background color from an indeterminate one. It returns false in WebKit/Gecko, which is clearly wrong, although that's a separate bug entirely.

I agree that returning something different for a mixed value is better than returning the value at the start of selection etc... We might want to talk about this on whatwg.org to get more feedback and eventually get standardized.  I also don't think changing it to return black is a good idea at this moment since that's a quite bit of behavior change that could be addressed in another bug.

> > Also, during the debugging, I found that CSSComputedStyleDeclaration always returns RGBA (RGB+Alpha value) for CSSPropertyBackgroundColor regardless of whether it's transparent or not.  So in the case of a node with background-color: transparent, we still get rgba(0,0,0,0).  It seems like we need to special case this value or modify the computed style to return CSSValueTransparent.  Does anyone know with whom I should talk about this issue?
> 
> I agree that this seems wrong. Again, it's worth checking what IE/Firefox do in this case.

Firefox returns transparent.  Need to check IE at home later.  Filed as the bug 42017.

[Ryosuke Niwa]

(In reply to comment #18)
> We need to be careful in these changes to not leak visited link information. :)

I assume this was intended for another bug.

[Ryosuke Niwa]

Created attachment 61136 [details]
fixed per Darin's comment

I moved the code to Editor::selectionCSSPropertyValue and made it to check alpha value instead of checking against RGBA(0, 0, 0, 0) per Darin's feedback.

[Ryosuke Niwa]

// FIXME: Rather than retrieving the style at the start of the current selection,
// we retrieve the style present throughout the selection.

should read
...
// we should retrieve the style present throughout the selection.

[Darin Adler]

Comment on attachment 61136 [details]
fixed per Darin's comment

> +        The bug was caused by the fact that the computed style of a transparent node indicates that
> +        the background color is rgba(0,0,0,0). While this is correct in the accordance to CSS2 because background-color
> +        is not inherited by default, this doesn't give the desired result for editing purposes.

I still don’t agree with this. I don’t think that querying some text on no background should go find the background that’s drawn behind that text. It seems like too high level a concept.

[Ryosuke Niwa]

Created attachment 61137 [details]
fixed style and FIXME

[Ryosuke Niwa]

(In reply to comment #23)
> (From update of attachment 61136 [details])
> > +        The bug was caused by the fact that the computed style of a transparent node indicates that
> > +        the background color is rgba(0,0,0,0). While this is correct in the accordance to CSS2 because background-color
> > +        is not inherited by default, this doesn't give the desired result for editing purposes.
> 
> I still don’t agree with this. I don’t think that querying some text on no background should go find the background that’s drawn behind that text. It seems like too high level a concept.

Let me give you an example.  Suppose we have

<div style="background: yellow;"><span id="test" style="text-decoration: underline;">hello, world</span></div>

When a user queries queryCommandValue("backColor") on #test, he/she wouldn't expect it to be rgba(0, 0, 0, 0) but expects it to be "yellow" or rgb(255, 255, 0) because that's what he/she sees.

Arguably, I'm not certain what we should do in the case where span was position:absolute or it overflowed beyond the parent element.  I added two failing tests in my patch that demonstrates this point.

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #18)
> > We need to be careful in these changes to not leak visited link information. :)
> 
> I assume this was intended for another bug.

It was intended for this bug. Exposing :visited state is a privacy concern. I'll look in more detail later. Its possible this bug has no privacy implications at all. :)

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > We need to be careful in these changes to not leak visited link information. :)
> > 
> > I assume this was intended for another bug.
> 
> It was intended for this bug. Exposing :visited state is a privacy concern. I'll look in more detail later. Its possible this bug has no privacy implications at all. :)

Oh, I see.  Sorry, I misunderstood you.  I should look into that.

[Ojan Vafai]

(In reply to comment #23)
> (From update of attachment 61136 [details])
> > +        The bug was caused by the fact that the computed style of a transparent node indicates that
> > +        the background color is rgba(0,0,0,0). While this is correct in the accordance to CSS2 because background-color
> > +        is not inherited by default, this doesn't give the desired result for editing purposes.
> 
> I still don’t agree with this. I don’t think that querying some text on no background should go find the background that’s drawn behind that text. It seems like too high level a concept.

execCommand and the queryCommand family of methods are a high level concept though. They should match what the user would expect to see in a rich-text toolbar/menu for the background color of the currently selected text. If they don't do that, then what use do they provide on top of getComputedStyle?

This brings us closer to the IE behavior, which is to try to match whatever background color the user sees selected. I think that's correct and matches what rich-text developers expect out of queryCommandValue.

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > We need to be careful in these changes to not leak visited link information. :)
> > 
> > I assume this was intended for another bug.
> 
> It was intended for this bug. Exposing :visited state is a privacy concern. I'll look in more detail later. Its possible this bug has no privacy implications at all. :)

I added a test case for this.

[Ryosuke Niwa]

(In reply to comment #23)
> (From update of attachment 61136 [details])
> > +        The bug was caused by the fact that the computed style of a transparent node indicates that
> > +        the background color is rgba(0,0,0,0). While this is correct in the accordance to CSS2 because background-color
> > +        is not inherited by default, this doesn't give the desired result for editing purposes.
> 
> I still don’t agree with this. I don’t think that querying some text on no background should go find the background that’s drawn behind that text. It seems like too high level a concept.

@Darin: do you still disagree with my point?  I've been trying to reach you on IRC but I haven't been able to find you.

[Darin Adler]

(In reply to comment #30)
> (In reply to comment #23)
> > (From update of attachment 61136 [details] [details])
> > > +        The bug was caused by the fact that the computed style of a transparent node indicates that
> > > +        the background color is rgba(0,0,0,0). While this is correct in the accordance to CSS2 because background-color
> > > +        is not inherited by default, this doesn't give the desired result for editing purposes.
> > 
> > I still don’t agree with this. I don’t think that querying some text on no background should go find the background that’s drawn behind that text. It seems like too high level a concept.
> 
> @Darin: do you still disagree with my point?  I've been trying to reach you on IRC but I haven't been able to find you.

I still think this could be a problem. When you ask the background color of an element, for example, you don’t get the color drawn behind, you only get the color of the element itself. Similarly, it seems strange that the editing functions are trying to find out what color will be displayed behind the text in cases where the text itself is not supplying the background.

While it’s true that execCommand and queryCommand are higher level than getComputedStyle, I’m not sure it’s so high level that it should be folding multiple elements in this fashion. The answer to why they exist is that they are the Internet Explorer editing API and we originally implemented it to be compatible with IE and work with the same websites.

But I may not be sufficiently expert here. I’m OK with making a change if we have consensus this will work well. Does any other browser do this sort of thing?

[Ryosuke Niwa]

(In reply to comment #31)
> I still think this could be a problem. When you ask the background color of an element, for example, you don’t get the color drawn behind, you only get the color of the element itself. Similarly, it seems strange that the editing functions are trying to find out what color will be displayed behind the text in cases where the text itself is not supplying the background.

But we often do this in editing code.  For example queryCommandState('bold') returns true for all descendes of b tag or node with font-weight: bold and so forth.

> But I may not be sufficiently expert here. I’m OK with making a change if we have consensus this will work well. Does any other browser do this sort of thing?

As I have implemented in #11, both Internet Explorer and Firefox retrieves the background color of ancestors so my patch will modify the WebKit's behavior to match other browsers.

[Darin Adler]

(In reply to comment #32)
> (In reply to comment #31)
> > I still think this could be a problem. When you ask the background color of an element, for example, you don’t get the color drawn behind, you only get the color of the element itself. Similarly, it seems strange that the editing functions are trying to find out what color will be displayed behind the text in cases where the text itself is not supplying the background.
> 
> But we often do this in editing code.  For example queryCommandState('bold') returns true for all descendes of b tag or node with font-weight: bold and so forth.

I see that case differently.

> > But I may not be sufficiently expert here. I’m OK with making a change if we have consensus this will work well. Does any other browser do this sort of thing?
> 
> As I have implemented in #11, both Internet Explorer and Firefox retrieves the background color of ancestors so my patch will modify the WebKit's behavior to match other browsers.

OK.

[Darin Adler]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > I still think this could be a problem. When you ask the background color of an element, for example, you don’t get the color drawn behind, you only get the color of the element itself. Similarly, it seems strange that the editing functions are trying to find out what color will be displayed behind the text in cases where the text itself is not supplying the background.
> > 
> > But we often do this in editing code.  For example queryCommandState('bold') returns true for all descendes of b tag or node with font-weight: bold and so forth.
> 
> I see that case differently.

Here’s why. The boldness is still part of the effective style of the text. It’s not just something the text is drawing on top of, it’s an actual attribute of the text.

The background color, however, is not, as I understand it. It’s just something already drawn under the text that the text shows up on top of. For example, the background could also be a video, or a gradient or a piece of an image. I don’t see how the color is different from those, and those definitely would not show up in the queryCommandSate.

But if this matches the other browsers, then I guess I have it wrong.

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > I still think this could be a problem. When you ask the background color of an element, for example, you don’t get the color drawn behind, you only get the color of the element itself. Similarly, it seems strange that the editing functions are trying to find out what color will be displayed behind the text in cases where the text itself is not supplying the background.
> > > 
> > > But we often do this in editing code.  For example queryCommandState('bold') returns true for all descendes of b tag or node with font-weight: bold and so forth.
> > 
> > I see that case differently.
> 
> Here’s why. The boldness is still part of the effective style of the text. It’s not just something the text is drawing on top of, it’s an actual attribute of the text.
> 
> The background color, however, is not, as I understand it. It’s just something already drawn under the text that the text shows up on top of. For example, the background could also be a video, or a gradient or a piece of an image. I don’t see how the color is different from those, and those definitely would not show up in the queryCommandSate.
> 
> But if this matches the other browsers, then I guess I have it wrong.

As a consumer, we use queryCommandValue for rich text editor functionalities. If this bug is fixed, we can for example show the background color state for any text in the editor similar to how we can show state for font, font size, bold, etc in our format bar.

For example, in Word, you can apply a yellow highlight to a word. The next time you put your cursor in the word and click to apply a highlight, it can show the currently applied background color on it in the palette.

[Darin Adler]

(In reply to comment #35)
> As a consumer, we use queryCommandValue for rich text editor functionalities. If this bug is fixed, we can for example show the background color state for any text in the editor similar to how we can show state for font, font size, bold, etc in our format bar.
> 
> For example, in Word, you can apply a yellow highlight to a word. The next time you put your cursor in the word and click to apply a highlight, it can show the currently applied background color on it in the palette.

In more sophisticated layouts I’d expect to be able to change the background color to transparent so I can see whatever’s behind the text. I want to be able to show that background color state in the editor.

[Ryosuke Niwa]

(In reply to comment #36)
> In more sophisticated layouts I’d expect to be able to change the background color to transparent so I can see whatever’s behind the text. I want to be able to show that background color state in the editor.

But for that purpose, the editor can use computedStyle since that would exactly tell them whether or not the node is transparent.  Since both Firefox and Internet Explorer traverse ancestors for queryCommandValue('BackColor'), we should do the same.

[Darin Adler]

Comment on attachment 61137 [details]
fixed style and FIXME

Looks good. A few problems.

> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR) {
> +        Color backgroundColor(value->getRGBA32Value());
> +        return backgroundColor.hasAlpha() && !backgroundColor.alpha();
> +    }

There is no reason to construct a Color here. You can check the alpha value in an RGBA 32-bit value with alphaChannel.

There is no reason to call hasAlpha here. hasAlpha is just "alpha != 256" and alpha is "alpha == 0" There's no reason to check both.

    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
        return !alphaChannel(value->getRGBA32Value());

> +    // FIXME: Rather than retrieving the style at the start of the current selection,
> +    // we retrieve the style present throughout the selection.
> +    RefPtr<CSSStyleDeclaration> selectionStyle = m_frame->selectionComputedStyle(nodeToRemove);

Is there a reason you need to comment on this, but not fix it here? Is there a test covering this?

> +        } while (isTransparent(selectionStyle.get()));

The function name isTransparent is unclear. I thin the intent is to check if the background color is transparent. That's not the same as saying a style "is transparent". I would call the function hasTransparentBackgroundColor because it doesn't even check if the background is transparent, just if the background *color* is transparent. You could have a non-transparent background image or a transparent background image. It also doesn't return true if the object has no background color at all, which seems like a mistake. Is the algorithm right for those cases?

>      TriState selectionHasStyle(CSSStyleDeclaration*) const;
> +    String selectionCSSPropertyValue(int);

The int here needs an argument name. It's not clear what an int here is, although I know it's a property ID.

I think it's unfortunate to add a new function here and have it be unclear if it means the property value at the start of the selection or throughout the selection. The old code didn't have this lack of clarity; it said "selection start" in its function name. We are adding the lack of clarity now. Lets not!

review- because I think this could be easily improved. Sorry it took me so long to get to reviewing this. There are a lot of patches!

[Ryosuke Niwa]

(In reply to comment #38)
> (From update of attachment 61137 [details])
> Looks good. A few problems.
> 
> > +    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR) {
> > +        Color backgroundColor(value->getRGBA32Value());
> > +        return backgroundColor.hasAlpha() && !backgroundColor.alpha();
> > +    }
> 
> There is no reason to construct a Color here. You can check the alpha value in an RGBA 32-bit value with alphaChannel.

Thanks for the info.  I wonder why I didn't realize that.

> There is no reason to call hasAlpha here. hasAlpha is just "alpha != 256" and alpha is "alpha == 0" There's no reason to check both.

Ah, good to know.

> > +    // FIXME: Rather than retrieving the style at the start of the current selection,
> > +    // we retrieve the style present throughout the selection.
> > +    RefPtr<CSSStyleDeclaration> selectionStyle = m_frame->selectionComputedStyle(nodeToRemove);
> 
> Is there a reason you need to comment on this, but not fix it here? Is there a test covering this?

Oops, that comment was incomplete.  I think the current implementation is probably correct for mac because TextEdit retrieves styles at the beginning of selection for styling / toggling purposes.  However, in all other platforms (as far as I know), we must determine using the style present throughout the selection.  For example, queryCommandValue('bold') should return false if the text has both bolded and unbolded text on non-mac platforms.  On mac, it should return true iff the start of selection is bolded.  But this is not specific to this bug and will have a large impact on the behaviors of other editing commands.  So I'd like to fix it in a separate patch.

> > +        } while (isTransparent(selectionStyle.get()));
> 
> The function name isTransparent is unclear. I thin the intent is to check if the background color is transparent. That's not the same as saying a style "is transparent". I would call the function hasTransparentBackgroundColor because it doesn't even check if the background is transparent, just if the background *color* is transparent. You could have a non-transparent background image or a transparent background image. It also doesn't return true if the object has no background color at all, which seems like a mistake. Is the algorithm right for those cases?

Renamed to hasTransparentBackgroundColor.  I think we should ignore background images for the purpose of finding the background color here because we can't reliably obtain "the color" of an image.  We always get background color property in this particular usage of hasTransparentBackgroundColor because computed style always has background color property.  But you're right that I should probably consider the case where someone calls hasTransparentBackgroundColor with a style declaration without background color.  Fixed.

> >      TriState selectionHasStyle(CSSStyleDeclaration*) const;
> > +    String selectionCSSPropertyValue(int);
> 
> The int here needs an argument name. It's not clear what an int here is, although I know it's a property ID.

Sure, fixed.

> I think it's unfortunate to add a new function here and have it be unclear if it means the property value at the start of the selection or throughout the selection. The old code didn't have this lack of clarity; it said "selection start" in its function name. We are adding the lack of clarity now. Lets not!

My intention was to move the problem of retrieving the style at the start of selection as supposed to style present throughout the selection into selectionCSSPropertyValue but I guess that'll clutter the code.  I renamed to selectionStartCSSPropertyValue and moved FIXME to valueStyle.

> review- because I think this could be easily improved. Sorry it took me so long to get to reviewing this. There are a lot of patches!

Thanks for the review.  It was really nice discussions that took place here.  We might want to summarize what we discussed here and post it on whatwg to standardize this behavior.

[Ryosuke Niwa]

Created attachment 65931 [details]
Patch

[Darin Adler]

Comment on attachment 65931 [details]
Patch

> -    return frame->selectionStartStylePropertyValue(propertyID);
> +    // FIXME: Rather than retrieving the style at the start of the current selection,
> +    // we should retrieve the style present throughout the selection for non-mac platforms.

It’s "Mac", not "mac".

[Ryosuke Niwa]

Committed r66431: <http://trac.webkit.org/changeset/66431>