386496 - Clicking on link in designMode document does follow that link now

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

See testcase, when clicking on the link, you should not be going to google.com.
I think this regressed with bug 237964.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

I don't really like spreading out the IsEditable checking for links like this, but it makes some sense (avoids us doing a bunch of things for links that won't work).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh... so what it the testcase, exactly?

Comment 3

[Martijn Wargers (dead)]

Sorry, I used the testcase from bug 287707 for that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow the main point of this patch.  Why do the IsEditable() checks need to be hoisted out of webshell?

For the rest, I think we should indeed be checking for editability in nsObjectFrame one way or another.

Is there a reason the new nsIContent method is non-virtual?  And non-hidden?  In particular, should it just be a static nsContentUtils method taking a principal as the first arg instead of being a member method, if the only use of |this| is for the principal?

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> I'm not sure I follow the main point of this patch.  Why do the IsEditable()
> checks need to be hoisted out of webshell?

They don't really need to move to fix this bug, but I didn't think they were in the right place in webshell. Plus, it seems like we can avoid running some code in the callers. For example, I don't think we should do the security check in TriggerLink if the node is editable.

> For the rest, I think we should indeed be checking for editability in
> nsObjectFrame one way or another.

Ok. I think we should actually stop plugins when they're contentEditable, which probably fixes the problem too.

> Is there a reason the new nsIContent method is non-virtual?  And non-hidden? 
> In particular, should it just be a static nsContentUtils method taking a
> principal as the first arg instead of being a member method, if the only use of
> |this| is for the principal?

Yes, that makes sense.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So what part of the patch actually fixes this bug?

Comment 7

[Peter Van der Beken [:peterv]]

The main fix is checking HasFlag(NODE_IS_EDITABLE) on the document, that means the document is in design mode. We used to just check HasFlag(NODE_IS_EDITABLE) on the node, but that just checks if the node is contentEditable. I'll add a comment to the IsEditable function to explain that.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Let me know if you prefer me to just do s/aContent->HasFlag(NODE_IS_EDITABLE)/aContent->IsEditable()/ in nsWebShell.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 271232 [details] [diff] [review]
v1.1

>Index: content/base/public/nsContentUtils.h
>+   * security check using aNode's principal.

aContent's, right?

>+   * @param aPresContext the pres context.

Document that this must be non-null and assert that in TriggerLink's implementation?

Also document that the URI must be non-null.

>+   * @param aClick whether this was a click or not (if false, it assumes you

"this method assumes you"

>Index: content/base/public/nsINode.h
>+  PRBool IsEditable() const;

Should this be virtual?  Or at least inline and expanding to either a virtual IsEditableExternal() or a non-virtual IsEditableInternal?

Or do we not expect anyone outside gklayout to call this?

>Index: content/base/src/nsContentUtils.cpp
>+  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();

You can nix the "nsContentUtils::" part.  Or just use sSecurityManager directly.

>Index: docshell/base/nsWebShell.cpp

I'd be happier with the IsEditable() calls here.  Belt and braces, and this is not exactly a critical path...

r+sr=bzbarsky with the nits picked.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Patch that was checked in

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Mochitest