Bug 20348 – Background color formatting lost on enter

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

[Julie Parent]

1. Go to midas demo: http://www.mozilla.org/editor/midasdemo/
2. Enter a word.
3. Select all.
4. Apply background color.
5. Place cursor at end of word.
6. Hit enter
7. Type a word.

Result: New text has no background color.
Expected result: Text has background color from first line.

Note: Font face, size, color, and bold/underline/italics are do not have this issue.

[Mark Rowe (bdash)]

<rdar://problem/6142319>

[Ryosuke Niwa]

Created attachment 31828 [details]
demonstrates the bug

[Ryosuke Niwa]

Created attachment 32684 [details]
fixes the bug by making more CSS properties inheritable.

[Justin Garcia]

Background color and text decoration are not inherited, so I think this needs to be fixed for editing specifically.  Also please add more details to your LayoutTests ChangeLog when your changes there go beyond adding a test case.

[Ryosuke Niwa]

(In reply to comment #4)
> Background color and text decoration are not inherited, so I think this needs
> to be fixed for editing specifically.

Thank you for pointing that out.  So then I can add new function copyEditableProperties to CSSComputedStyleDeclaration, and replace all instances of copyInheritableProperties in editing code by copyEditableProperties.  Do you think this is sensible approach?

[Darin Adler]

Comment on attachment 32684 [details]
fixes the bug by making more CSS properties inheritable.

We have to be quite careful about this. I do not believe that text-decoration is inheritable. Instead it simply looks as if it is, since the text decoration on the parent overwrites the child. I also am concerned that some fo these tests are showing explicit background-color values in cases where the color already matches its parent.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 32684 [details])
> We have to be quite careful about this. I do not believe that text-decoration
> is inheritable. Instead it simply looks as if it is, since the text decoration
> on the parent overwrites the child. I also am concerned that some fo these
> tests are showing explicit background-color values in cases where the color
> already matches its parent.

Thank you for your response.  You're right, somehow I didn't connect the world
"inheritable" to that of CSS properties.  Anyway, I searched WebCore and it
turned out that copyInheritableProperties is only used under editing directory.
 So perhaps we can rename that function and start copying background color and
text decorations.  And I think spans having explicit background-color values is
another bug.  I've been a similar bug report somewhere.

[Ryosuke Niwa]

I did further investigation, and it turned out that inheritableProperties ([e-mail]) is used in only two functions: removeComputedInheritablePropertiesFrom and copyInheritableProperties.  But it turned out that removeComputedInheritablePropertiesFrom is NOT used anywhere.  And copyInheritableProperties is used only in WebCore/editing/.  So it's safe to say that inheritableProperties doesn't need to represent CSS inheritable properties.

At this point, we have three choices.
1. Use my patch.
2. My patch + rename removeComputedInheritablePropertiesFrom and copyInheritableProperties
3. We add another function like copyEditableProperties, and gradually replace them (it's used in 6 editing files)

Comments?

[Darin Adler]

I prefer (2).

[Ryosuke Niwa]

(In reply to comment #9)
> I prefer (2).

Hi Darin, I talked with Ojan about this further, and he told me that he talked about CSSComputedStyleDeclaration with Mitz, and Mitz had an intention of separating it into two classes that are purely computed styles and editing styles instead of us having one class that are multi-purpose.

But then I realized that we can take copyInheritableProperties out of the class and make it a function.  To achieve this, I still need to add getRenderStyleOfNode that returns m_node->computedStyle() but I don't see why that would be a problem.  Do you think this is a good approach?

[Darin Adler]

(In reply to comment #10)
> To achieve this, I still need to add getRenderStyleOfNode that returns m_node->computedStyle()

I’m sorry, I don’t understand this part. Where do you need to add this, and why? Why does this function’s name have "get" in it?

[Ryosuke Niwa]

(In reply to comment #11)
> I’m sorry, I don’t understand this part. Where do you need to add this, and
> why? Why does this function’s name have "get" in it?

http://trac.webkit.org/browser/trunk/WebCore/css/CSSComputedStyleDeclaration.cpp
1503	        if (!m_node->computedStyle()->textFillColor().isValid())
1504	            style->removeProperty(CSSPropertyWebkitTextFillColor, ec);
1505	        if (!m_node->computedStyle()->textStrokeColor().isValid())
1506	            style->removeProperty(CSSPropertyWebkitTextStrokeColor, ec);

Because this code, my function still need to be able to access to the render style of the node.

so I added a new method to CSSComputedStyleDeclaration:
RenderStyle* renderStyleOfNode()
{
   ASSERT(m_node);
    return m_node->computedStyle();
}

[Ryosuke Niwa]

Created attachment 32820 [details]
fixes the bug and renames copyInheritableProperties to copyPropertiesForEditing and removes removeComputedInheritablePropertiesFrom

This is the patch for (2).

[Eric Seidel (no email)]

Comment on attachment 32820 [details]
fixes the bug and renames copyInheritableProperties to copyPropertiesForEditing and removes removeComputedInheritablePropertiesFrom

This name is not sufficiently clear:
+static const int propertiesForEditing[] = {

What are these actually used for?

Do we have tests for both of these?
     // Properties for editing
 1472     CSSPropertyTextDecoration,
 1473     CSSPropertyBackgroundColor,
14701474 };

It seems this could break the optimization in:
handleStyleSpansBeforeInsertion
(which may already be broken)

Seems uses of this function are inconsistent in their needs.  I think this is just a bad function which dates from before time:
(I stopped digging after http://trac.webkit.org/changeset/7314)

I think we should just deprecate this function and make a new (somewhat copied) one which works for your uses.  All the callers of this need to change, but I'm not gonna ask you to do that.

[Ryosuke Niwa]

It turned out that making new version of this function and using it at specific places causes a lot of tests to fail because we're taking the difference of the style we're applying and the copyInheritableProperties of computed style in many places where we apply the style.

[Ryosuke Niwa]

Created attachment 34034 [details]
fixes the bug and adds new functions to replace copyInheritableProperties

[Eric Seidel (no email)]

Comment on attachment 34034 [details]
fixes the bug and adds new functions to replace copyInheritableProperties

Intent:
static const int editingStyleProperties[] = {
 307 CSSPropertyBackgroundColor,
 308 

Also, needs some sort of comment to explain what "editing style" is.

We could use an enum to make typing style includes more readable at the callsite:

editingStyleAtPosition(position, IncludeTypingStyle);

You would do that by making a 2 state enum:
enum ShouldIncludeTypingStyle {
    IncludeTypingStyle,
    IgnoreTypingStyle
};

You could even default the value to one or the other to whatever the more common behavior is.

I think you probably mean "removeStylesFromNode" instead of "removeStyleOfNode".

We could even consider putting this stuff in a new TypingStyle.h/cpp or EditingStyle.h/cpp file.  Especially if you think we're going to have more logic like these.

[Ryosuke Niwa]

(In reply to comment #17)
> I think you probably mean "removeStylesFromNode" instead of
> "removeStyleOfNode".

Since we're removing style coming from a particular node instead of removing style on a note, should call it removeStylesAddedByNode as you suggested on chat.

[Ryosuke Niwa]

Created attachment 34222 [details]
fixed per comment & removes deprecatedCopyInheritableProperties

[Ryosuke Niwa]

Created attachment 34336 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 34337 [details]
fixed the WebCore/ChangeLog

[Justin Garcia]

Comment on attachment 34337 [details]
fixed the WebCore/ChangeLog

r=me

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/47008.