Bug 20348 - Background color formatting lost on enter
Summary: Background color formatting lost on enter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC All
: P3 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, NeedsReduction
Depends on: 27325 28057
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-11 14:36 PDT by Julie Parent
Modified: 2009-08-10 15:29 PDT (History)
6 users (show)

See Also:


Attachments
demonstrates the bug (1.00 KB, text/html)
2009-06-24 19:20 PDT, Ryosuke Niwa
no flags Details
fixes the bug by making more CSS properties inheritable. (17.11 KB, patch)
2009-07-13 15:28 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug and renames copyInheritableProperties to copyPropertiesForEditing and removes removeComputedInheritablePropertiesFrom (32.77 KB, patch)
2009-07-15 16:20 PDT, Ryosuke Niwa
eric: review-
Details | Formatted Diff | Diff
fixes the bug and adds new functions to replace copyInheritableProperties (26.53 KB, patch)
2009-08-03 19:23 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per comment & removes deprecatedCopyInheritableProperties (30.83 KB, patch)
2009-08-06 13:23 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (6.17 KB, patch)
2009-08-07 15:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed the WebCore/ChangeLog (6.19 KB, patch)
2009-08-07 15:27 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 2008-08-11 14:36:08 PDT
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.
Comment 1 Mark Rowe (bdash) 2008-08-11 17:07:17 PDT
<rdar://problem/6142319>
Comment 2 Ryosuke Niwa 2009-06-24 19:20:16 PDT
Created attachment 31828 [details]
demonstrates the bug
Comment 3 Ryosuke Niwa 2009-07-13 15:28:25 PDT
Created attachment 32684 [details]
fixes the bug by making more CSS properties inheritable.
Comment 4 Justin Garcia 2009-07-13 18:52:19 PDT
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.
Comment 5 Ryosuke Niwa 2009-07-14 11:43:29 PDT
(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?
Comment 6 Darin Adler 2009-07-14 11:54:50 PDT
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.
Comment 7 Ryosuke Niwa 2009-07-14 12:37:23 PDT
(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.
Comment 8 Ryosuke Niwa 2009-07-15 11:28:14 PDT
I did further investigation, and it turned out that inheritableProperties (1446@CSSComputedStyleDeclaration.cpp) 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?
Comment 9 Darin Adler 2009-07-15 11:30:44 PDT
I prefer (2).
Comment 10 Ryosuke Niwa 2009-07-15 12:00:37 PDT
(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?
Comment 11 Darin Adler 2009-07-15 12:03:07 PDT
(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?
Comment 12 Ryosuke Niwa 2009-07-15 12:08:59 PDT
(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();
}
Comment 13 Ryosuke Niwa 2009-07-15 16:20:49 PDT
Created attachment 32820 [details]
fixes the bug and renames copyInheritableProperties to copyPropertiesForEditing and removes removeComputedInheritablePropertiesFrom

This is the patch for (2).
Comment 14 Eric Seidel (no email) 2009-07-15 16:59:55 PDT
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.
Comment 15 Ryosuke Niwa 2009-08-02 01:04:53 PDT
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.
Comment 16 Ryosuke Niwa 2009-08-03 19:23:15 PDT
Created attachment 34034 [details]
fixes the bug and adds new functions to replace copyInheritableProperties
Comment 17 Eric Seidel (no email) 2009-08-05 14:05:22 PDT
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.
Comment 18 Ryosuke Niwa 2009-08-05 14:14:30 PDT
(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.
Comment 19 Ryosuke Niwa 2009-08-06 13:23:36 PDT
Created attachment 34222 [details]
fixed per comment & removes deprecatedCopyInheritableProperties
Comment 20 Ryosuke Niwa 2009-08-07 15:25:07 PDT
Created attachment 34336 [details]
fixes the bug
Comment 21 Ryosuke Niwa 2009-08-07 15:27:24 PDT
Created attachment 34337 [details]
fixed the WebCore/ChangeLog
Comment 22 Justin Garcia 2009-08-10 11:04:26 PDT
Comment on attachment 34337 [details]
fixed the WebCore/ChangeLog

r=me
Comment 23 Ryosuke Niwa 2009-08-10 15:29:17 PDT
Landed in http://trac.webkit.org/changeset/47008.