Bug 27325 – copyInheritableProperties and removeComputedInheritablePropertiesFrom should be deprecated

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

[Ryosuke Niwa]

removeComputedInheritablePropertiesFrom is not used anywhere and copyInheritableProperties is used across the editing code but very inconsistently.  copyInheritableProperties is poorly named considering the usage of the function.  copyInheritableProperties should be deprecated and callers should use a better named function.

Below are just examples of how copyInheritableProperties is used:

[e-mail]
    RefPtr<CSSMutableStyleDeclaration> styleAtInsertionPos = rangeCompliantEquivalent(insertionPos).computedStyle()->copyInheritableProperties();
    String styleText = styleAtInsertionPos->cssText();
    
    if (styleText == static_cast<Element*>(sourceDocumentStyleSpan)->getAttribute(styleAttr)) {

Here, it's used to compare the styles of the source and destination.


[e-mail]
    // Get the default style for this editable root, it's the style that we'll give the
    // content that we're operating on.
    Node* root = frame->selection()->rootEditableElement();
    RefPtr<CSSMutableStyleDeclaration> defaultStyle = computedStyle(root)->copyInheritableProperties();

Here, it's used to copy styles.


[e-mail]
    // Figure out the typing style in effect before the delete is done.
    RefPtr<CSSComputedStyleDeclaration> computedStyle = positionBeforeTabSpan(m_selectionToDelete.start()).computedStyle();
    m_typingStyle = computedStyle->copyInheritableProperties();

And here, it's used to retrieve the typing style.

[Ryosuke Niwa]

Eric and I figured out that the last time any significant change was made to this function was http://trac.webkit.org/changeset/7314.

[Ryosuke Niwa]

It seems to me that while there are several usages of this function, we can categorize it into two:

1. Preserving styles - e.g. you want to preserve styles while editing

Because we almost always remove redundant styles when reapplying the style, we should make a function that takes the destination position, and applies the minimum styles automatically.

2. Typing styles - we set typingStyle

A lot of problems are caused by typing styles (e.g. highlight is lost in various cases).  It may make sense to make a new class, say, TypyingStyle class that centralizes code related to typing style.

Any alternative ideas / suggestions?

[Ryosuke Niwa]

Created attachment 32825 [details]
removes removeComputedInheritablePropertiesFrom and renames copyInheritableProperties

[Ryosuke Niwa]

Created attachment 32826 [details]
Deprecates copyInheritableProperties and removes removeComputedInheritablePropertiesFrom

Added more detailed comments

[Justin Garcia]

This seems reasonable except that preserving styles will still rely on the concept of inheritable properties.  Will it just not use this method that you're deprecating?

[Ryosuke Niwa]

(In reply to comment #5)
> This seems reasonable except that preserving styles will still rely on the
> concept of inheritable properties.  Will it just not use this method that
> you're deprecating?

I think when we preserve style during editing operations, we also want to preserve background color and text decorations, which are not inheritable properties of CSS.  So the name is still confusing.

[Eric Seidel (no email)]

Comment on attachment 32826 [details]
Deprecates copyInheritableProperties and removes removeComputedInheritablePropertiesFrom

Seems fine.

[Ryosuke Niwa]

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