Bug 25316 – Outdenting a line inside a blockquote tag does nothing

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

[Marcos Almeida]

1) go to http://www.mozilla.org/editor/midasdemo/
2) check View HTML
3) enter "<blockquote>one<br>two</blockquote>"
4) uncheck View HTML
5) put the cursor at the beginning of "two"
6) press the outdent button
  --> nothing happens. the line should have been outdented (e.g. removed from inside the blockquote tags)

[Ryosuke Niwa]

Created attachment 32540 [details]
fixes the bug

[Ryosuke Niwa]

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).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Ryosuke Niwa]

Created attachment 32601 [details]
removes isIndentBlockquote as requested

[Eric Seidel (no email)]

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+

[Ryosuke Niwa]

Created attachment 32635 [details]
fixes the bug

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

Comment on attachment 32635 [details]
fixes the bug

Looks sane enough to me.

[Ryosuke Niwa]

This patch doesn't have the rebaselined expected results for qt platform.  Please rebaseline it as needed.

[Julie Parent]

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