Bug 71357 - Web Inspector: Cannot edit elements commented with <!--
Summary: Web Inspector: Cannot edit elements commented with <!--
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-02 04:52 PDT by Alexander Pavlov (apavlov)
Modified: 2011-11-07 02:51 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] Suggested solution (11.05 KB, patch)
2011-11-03 06:05 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed (14.67 KB, patch)
2011-11-03 10:09 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed - 2 (15.09 KB, patch)
2011-11-07 00:41 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-11-02 04:52:14 PDT
What steps will reproduce the problem?
1. Navigate to phrack.com/issues.html
2. Open developer tools 
3. Right click on super secret hidden Kiwicon endorsement


What is the expected result?

4. Get "Edit as HTML" option
5. Uncomment

What happens instead?

4. No option to edit, only Inspect Element and Word Wrap.

Please provide any additional information below. Attach a screenshot if
possible.

If I comment out any element in this fashion I cannot uncomment it via the dev tools, i have to reload the page.

I don't see why the dev tools can't edit commented elements. That doesnt make sense.

Upstreaming http://code.google.com/p/chromium/issues/detail?id=92605
Comment 1 Alexander Pavlov (apavlov) 2011-11-03 06:05:24 PDT
Created attachment 113471 [details]
[PATCH] Suggested solution
Comment 2 Pavel Feldman 2011-11-03 06:57:49 PDT
Comment on attachment 113471 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=113471&action=review

r- for two branches in setOuterHTML

> LayoutTests/inspector/elements/edit-dom-actions.html:165
> +        if (typeof dataNode === "string")

You should use different method for comment nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:772
> +        RefPtr<DocumentFragment> fragment = DocumentFragment::create(document);

You should use this branch for both cases.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:776
> +            return;

Assign errorString here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:781
> +            return;

ditto

> Source/WebCore/inspector/InspectorDOMAgent.cpp:791
> +        if (ec)

ditto

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1061
> +    _populateCommentContextMenu: function(contextMenu, commentNode)

You should call this method from _populateTagContextMenu (and call it _populateNodeContextMenu). Should also work for Text nodes.
Comment 3 Alexander Pavlov (apavlov) 2011-11-03 10:09:21 PDT
Created attachment 113515 [details]
[PATCH] Comments addressed
Comment 4 Pavel Feldman 2011-11-05 09:05:47 PDT
Comment on attachment 113515 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=113515&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:751
>      HTMLElement* element = assertHTMLElement(errorString, nodeId);

If user passes TextNode id into this method, he will get an error message saying that it should always be id of the Element. But we do support nodes (see comment above). You should either support more node types and remove this assertion or not call getOuterHTML for non-Element nodes.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:772
> +    if (document->isHTMLDocument())

You could put this check earlier. Can this happen at all?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:787
> +        requiresTotalUpdate = htmlElement->tagName() == "HTML" || htmlElement->tagName() == "BODY" || htmlElement->tagName() == "HEAD";

no need to cast, tagName() is nodeName() for elements.
Comment 5 Alexander Pavlov (apavlov) 2011-11-07 00:41:37 PST
Created attachment 113832 [details]
[PATCH] Comments addressed - 2
Comment 6 WebKit Review Bot 2011-11-07 01:59:44 PST
Comment on attachment 113832 [details]
[PATCH] Comments addressed - 2

Clearing flags on attachment: 113832

Committed r99401: <http://trac.webkit.org/changeset/99401>
Comment 7 WebKit Review Bot 2011-11-07 01:59:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Yuta Kitamura 2011-11-07 02:51:44 PST
Committed r99402: <http://trac.webkit.org/changeset/99402>