Bug 30836

Summary: Creating a link when selecting multiple nodes creates multiple links
Product: WebKit Reporter: Marcos Almeida <marcosalmeida>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: enrica, jparent, ojan, rniwa, tony
Priority: P2 Keywords: GoogleBug
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.mozilla.org/editor/midasdemo/
Attachments:
Description Flags
initial patch
none
fixed per Tony's comment tony: review+

Marcos Almeida
Reported 2009-10-27 14:52:44 PDT
1. Go to http://www.mozilla.org/editor/midasdemo/ 2. type "abc" 3. select "b" and press the italic button on the second toolbar 4. press ctrl-a to select all 5. press the create link button (the globe icon with a chain link, on the first toolbar) 6. type abc in the dialog that pops up and press ok 7. click the "view html source" checkbox at the bottom to see the generated html Expected: <a href="abc">a<i>b</i>c</a> Actual: <a href="abc">a</a><i><a href="abc">b</a></i><a href="abc">c</a> This affects Google properties.
Attachments
initial patch (31.75 KB, patch)
2010-08-24 18:55 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixed per Tony's comment (34.75 KB, patch)
2010-08-25 13:33 PDT, Ryosuke Niwa 		tony: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 2 2010-08-24 18:55:11 PDT
Created attachment 65356 [details] initial patch
Tony Chang
Comment 3 2010-08-25 12:12:28 PDT
Comment on attachment 65356 [details] initial patch > Index: WebCore/editing/ApplyStyleCommand.cpp > + bool operator==(const StyleChange& other) > + { > + return m_cssStyle == other.m_cssStyle I am worried that this will regress someday when someone adds a new member, but I don't have a good suggestion on how to avoid this. > - while (sibling && sibling != pastEndNode && (!sibling->isElementNode() || sibling->hasTagName(brTag)) && !isBlock(sibling)) { > + StyleChange startChange(style, Position(node, 0)); > + while (sibling && sibling != pastEndNode && !sibling->contains(pastEndNode)) { > + if (isBlock(sibling) && !sibling->hasTagName(brTag)) > + break; > + if (StyleChange(style, Position(sibling, 0)) != startChange) > + break; Was there a reason to move conditions into separate if statements? Seems like these could be merged with the while condition. > + RefPtr<StyledElement> styledElement; > + if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName())) > + styledElement = static_cast<StyledElement*>(current); Does |styledElement| need to be a RefPtr? > Index: WebCore/editing/ApplyStyleCommand.h > - void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end); > + void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end, bool shouldAddStyledElement = true); Can we use an enum instead of a bool? Can you also add a test for anchors with inline styles.
Ryosuke Niwa
Comment 4 2010-08-25 13:33:28 PDT
Created attachment 65469 [details] fixed per Tony's comment
Ryosuke Niwa
Comment 5 2010-08-25 13:34:45 PDT
(In reply to comment #3) > (From update of attachment 65356 [details]) > > Index: WebCore/editing/ApplyStyleCommand.cpp > > + bool operator==(const StyleChange& other) > > + { > > + return m_cssStyle == other.m_cssStyle > > I am worried that this will regress someday when someone adds a new member, but I don't have a good suggestion on how to avoid this. I know. I spent some time thinking about it but couldn't come up with a better way either. > > - while (sibling && sibling != pastEndNode && (!sibling->isElementNode() || sibling->hasTagName(brTag)) && !isBlock(sibling)) { > > + StyleChange startChange(style, Position(node, 0)); > > + while (sibling && sibling != pastEndNode && !sibling->contains(pastEndNode)) { > > + if (isBlock(sibling) && !sibling->hasTagName(brTag)) > > + break; > > + if (StyleChange(style, Position(sibling, 0)) != startChange) > > + break; > > Was there a reason to move conditions into separate if statements? Seems like these could be merged with the while condition. Merged. > > + RefPtr<StyledElement> styledElement; > > + if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName())) > > + styledElement = static_cast<StyledElement*>(current); > > Does |styledElement| need to be a RefPtr? Yes, because extractInlineStyleToPushDown may remove "current" in which case it may be destroyed if we didn't kept a pointer. > > Index: WebCore/editing/ApplyStyleCommand.h > > - void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end); > > + void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end, bool shouldAddStyledElement = true); > > Can we use an enum instead of a bool? Fixed. > Can you also add a test for anchors with inline styles. Good thing we talked about this. There were a couple of bugs revealed by new test cases.
Ryosuke Niwa
Comment 6 2010-08-25 14:15:47 PDT
Committed r66040: <http://trac.webkit.org/changeset/66040>
Note You need to log in before you can comment on or make changes to this bug.