| Summary: | WebKit does not merge text decorations in the typing style and the selected element properly | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, eric, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | 55338, 55452 | ||||||
| Bug Blocks: | 55339, 49956 | ||||||
| Attachments: |
|
||||||
Created attachment 84201 [details]
fixes the bug
Comment on attachment 84201 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=84201&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:1284 > + if (node->isHTMLElement() && static_cast<HTMLElement*>(node)->inlineStyleDecl()) { > + newInlineStyle = style->copy(); > + newInlineStyle->mergeInlineStyleOfElement(static_cast<HTMLElement*>(node)); > } Why is this code specific to HTMLElement? Can’t this be done on a custom element that is technically not an HTML element? This code would treat <subsection>, for example, differently from <section>. I am not sure I understand why we have so much code specific to HTMLElement. > Source/WebCore/editing/EditingStyle.cpp:714 > + bool shouldAddTextDecorations = false; > + > + RefPtr<CSSValue> value; I think it would be clearer to combine these two local variables. The fact that “value” is specifically a text decorations value is unclear given the current name, and I think that the value being a null pointer would be just as good an indicator as the boolean if it had a good name. Or maybe not. (In reply to comment #2) > (From update of attachment 84201 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84201&action=review > > > Source/WebCore/editing/ApplyStyleCommand.cpp:1284 > > + if (node->isHTMLElement() && static_cast<HTMLElement*>(node)->inlineStyleDecl()) { > > + newInlineStyle = style->copy(); > > + newInlineStyle->mergeInlineStyleOfElement(static_cast<HTMLElement*>(node)); > > } > > Why is this code specific to HTMLElement? Can’t this be done on a custom element that is technically not an HTML element? This code would treat <subsection>, for example, differently from <section>. I am not sure I understand why we have so much code specific to HTMLElement. It's only for historical reasons. We can fix in a separate patch. Is there a particular test case you're thinking of? It'll be good to have a regression test. > > Source/WebCore/editing/EditingStyle.cpp:714 > > + bool shouldAddTextDecorations = false; > > + > > + RefPtr<CSSValue> value; > > I think it would be clearer to combine these two local variables. The fact that “value” is specifically a text decorations value is unclear given the current name, and I think that the value being a null pointer would be just as good an indicator as the boolean if it had a good name. That's an excellent point. I'd just set value = 0 if it was value && !value->isValueList(). Thanks for the review. I'll be following up on the bug 51389 now. Committed r80060: <http://trac.webkit.org/changeset/80060> http://trac.webkit.org/changeset/80060 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/viewport/viewport-112.html fast/viewport/viewport-121.html fast/viewport/viewport-122.html fast/viewport/viewport-125.html fast/viewport/viewport-129.html fast/viewport/viewport-35.html fast/viewport/viewport-46.html fast/viewport/viewport-52.html fast/viewport/viewport-53.html fast/viewport/viewport-54.html fast/viewport/viewport-55.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-83.html fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml http/tests/security/xss-DENIED-xsl-document-redirect.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/xmlviewer/dumpAsText/wml.xml http://trac.webkit.org/changeset/80061 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/viewport/viewport-112.html fast/viewport/viewport-121.html fast/viewport/viewport-122.html fast/viewport/viewport-125.html fast/viewport/viewport-129.html fast/viewport/viewport-35.html fast/viewport/viewport-46.html fast/viewport/viewport-52.html fast/viewport/viewport-53.html fast/viewport/viewport-54.html fast/viewport/viewport-55.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-83.html fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml http/tests/security/xss-DENIED-xsl-document-redirect.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/xmlviewer/dumpAsText/wml.xml |
When the typing style and the element that contains the caret has conflicting text decoration values, WebKit does not merge the styles when style is queried. e.g. When we have a caret in <div style="text-decoration: underline;">hello</div>, and the typing style only has line-through, queryCommandState("underline") returns false