Summary: | Outdenting a line inside a blockquote tag does nothing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcos Almeida <marcosalmeida> | ||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, jparent, justin.garcia, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Windows XP | ||||||||||
Attachments: |
|
Description
Marcos Almeida
2009-04-21 16:14:18 PDT
Created attachment 32540 [details]
fixes the bug
Comment on attachment 32540 [details]
fixes the bug
The patch is large only because I had to rebaseline a pixel test. The actual change is very small (<10 lines).
Ok, this was added in: http://trac.webkit.org/changeset/17143 It seems that Justin/Levi were trying to add the ability for Mail.app to style blockquotes added by WebKit. I think an unintentional side-effect of this change was that WebKit was changed to no longer to outdent non-webkit blockquotes. So I guess we could call this bug a REGRESSION(17143) although it seems no one noticed until much later. Comment on attachment 32540 [details] fixes the bug I think we should remove this function: 6060 static bool isIndentBlockquote(const Node* node) and just use tag-based detection of blockquotes like was the case pre-r17143 Why is: LayoutTests/platform/mac/editing/execCommand/outdent-selection-expected.txt changing? Please explain in your ChagneLog. Please remove boilerplate from your ChangeLogs: DETAILED DESCRIPTION OF THE CHANGES GOES HERE. (OOPS!) SEE: 9 http://webkit.org/coding/contributing.html FOR MORE INFORMATION You soudl explain all your added tests and changes to tests in your ChangeLogs. Please explain this change: if (enclosingBlockFlow != enclosingNode) 275 splitBlockquoteNode = splitTreeToNode(enclosingBlockFlowElement(visibleStartOfParagraph), enclosingNode, true); 271 splitBlockquoteNode = splitTreeToNode(enclosingBlockFlow, enclosingNode, true); 272 else { 273 splitElement(static_cast<Element*>(enclosingBlockFlow), visibleStartOfParagraph.deepEquivalent().node()); 274 splitBlockquoteNode = enclosingBlockFlow; 275 } in your ChangeLog. Created attachment 32601 [details]
removes isIndentBlockquote as requested
Comment on attachment 32601 [details]
removes isIndentBlockquote as requested
Why is it OK to remove the ! check here?
- if (isIndentBlockquote(splitPointParent)
- && !isIndentBlockquote(splitPoint)
+ if (splitPointParent->hasTagName(blockquoteTag)
Style:
+ else
+ splitElement(static_cast<Element*>(enclosingNode), visibleStartOfParagraph.deepEquivalent().node());
+ // We split the blockquote at where we start outdenting.
comment would go before the splt, and the whole thing would be in { }
In general this looks fine. You're a committer now, so you could just commit this with edits, but I'd liek to see your answer to the first question before I r+
Created attachment 32635 [details]
fixes the bug
(In reply to comment #6) > (From update of attachment 32601 [details]) > Why is it OK to remove the ! check here? > - if (isIndentBlockquote(splitPointParent) > - && !isIndentBlockquote(splitPoint) > + if (splitPointParent->hasTagName(blockquoteTag) Good catch. I didn't realize that I deleted a line. Fixed. > Style: > + else > + splitElement(static_cast<Element*>(enclosingNode), > visibleStartOfParagraph.deepEquivalent().node()); > + // We split the blockquote at where we start outdenting. > comment would go before the splt, and the whole thing would be in { } Fixed. Comment on attachment 32635 [details]
fixes the bug
Looks sane enough to me.
This patch doesn't have the rebaselined expected results for qt platform. Please rebaseline it as needed. Landed in http://trac.webkit.org/changeset/45886 |