274885 - JS errors Ctrl/Shift+Left Clicking and Right Clicking links in P3P summary

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[[e-mail]]

We need to decide whether or not to handle these clicks; currently they generate
an error that XPCNativeWrapper is not defined; adding XPCNativeWrapper generates
an error that getBrowser() doesn't support addTab. The latter was caused by the
patch to bug 265683 renaming getSummaryBrowser() to getBrowser().
Note that the 1.4 and 1.7 branches have had XPCNativeWrapper added.

Comment 1

[Ian Neal]

Also happens on Shift+left click and right click. Updating summary.

Comment 2

[Ian Neal]

From bug 265683
* Renames getSummaryBrowser back to getBrowser so Text Zoom works

One of the questions we should ask is do we assume that sites that have a P3P
summary will write them in such a way that links that need to open a new window
are targetted correctly?

The captureContentClick function could be used if we wanted to screen out all
but simple left clicks with something like the following inserted:

  if (aEvent.type != "click" || aEvent.button != 0 ||
      aEvent.ctrlKey || aEvent.metaKey || aEvent.shiftKey)
    return true;

Comment 3

[Ian Neal]

The above still allowed left clicks on non-links to generate a JS error, the
following fixes that but we may want to replicate some of the code in the
hrefForClickEvent function in contentAreaClick.js for a proper fix:

  if ((aEvent.type == "click" && aEvent.target.localName.toLowerCase() != "link") ||
      aEvent.type != "click" || aEvent.button != 0 ||
      aEvent.ctrlKey || aEvent.metaKey || aEvent.shiftKey)
    return true;

Comment 4

[[e-mail]]

(In reply to comment #2)
> From bug 265683
> * Renames getSummaryBrowser back to getBrowser so Text Zoom works
The other way to fix this would be to redefine getMarkupDocumentViewer currently
defined by browser.js (why do we have browser.js?)

>One of the questions we should ask is do we assume that sites that have a P3P
>summary will write them in such a way that links that need to open a new
>window are targetted correctly?
We can add a URIContentListener to the docShell to fix this, a bit like the one
in help, but for all links, not based on the target href.

Comment 5

[Ian Neal]

It looks like we're using browser.js for Find and possibly ToolTips

Comment 6

[[e-mail]]

Well, we don't appear to have any tooltips, but we need it for Find; it looks as
if we'd be better off not handling modified clicks at all...

Comment 7

[Ian Neal]

Attachment: NativeWrapper and clicks patch v0.1

Something like this?

Comment 8

[[e-mail]]

Comment on attachment 168894 [details] [diff] [review]
NativeWrapper and clicks patch v0.1

>+  if (!(aEvent.target instanceof HTMLAnchorElement) || aEvent.button != 0 ||
>+      aEvent.altKey || aEvent.ctrlKey || aEvent.metaKey || aEvent.shiftKey)
>+    return true;
This doesn't seem to leave anything for contentAreaClick to do (note that
contentAreaClick does not handle unmodified left clicks). If that's the intent
then we can remove the relevant code (including XPCNativeWrapper).

Comment 9

[Ian Neal]

Attachment: Simple left click only patch v0.2

Comment 10

[[e-mail]]

Comment on attachment 168905 [details] [diff] [review]
Simple left click only patch v0.2

OK, so can we clean up the .xul too?

Comment 11

[Ian Neal]

Removing contentAreaClick.js and the DragAndDrop stuff?

Comment 12

[[e-mail]]

(In reply to comment #11)
>Removing contentAreaClick.js and the DragAndDrop stuff?
Good point, that doesn't belong either.

Comment 13

[Ian Neal]

Attachment: Left click and XUL tidyup patch v0.2a

Comment 14

[Daniel Brooks [:db48x]]

Comment on attachment 169048 [details] [diff] [review]
Left click and XUL tidyup patch v0.2a

looks good, r=db48x

Comment 15

[[e-mail]]

Comment on attachment 169048 [details] [diff] [review]
Left click and XUL tidyup patch v0.2a

Seems to me that contentAreaUtils.js can go too.

Comment 16

[[e-mail]]

OK, so in fact we could reimplement Find the way Mail does it (see MsgFind in
mailWindowOverlay.js) which would allows us to get rid of browser.js and thus
getBrowser(), should anyone really want to be able to ctrl+click a link.

Note that the .xsl used to generate the policy viewer content automatically
generates target="_blank" on external links.

Comment 17

[Ian Neal]

contentAreaUtils.js is used by p3pSummarySavePage function in p3pSummary.js
(saveDocument).
I will investigate removing browser.js

Comment 18

[[e-mail]]

(In reply to comment #17)
>contentAreaUtils.js is used by p3pSummarySavePage function in p3pSummary.js
>(saveDocument).
I only see persistor.saveDocument
>I will investigate removing browser.js
No point, the comment was only for someone wanting to restore ctrl+clicks.

Comment 19

[Ian Neal]

Attachment: Left click and more XUL tidyup patch v0.2b

Carrying forward r/sr

Comment 20

[Ian Neal]

Comment on attachment 169212 [details] [diff] [review]
Left click and more XUL tidyup patch v0.2b

Checking in p3pSummary.xul;
/cvsroot/mozilla/extensions/p3p/resources/content/p3pSummary.xul,v  <-- 
p3pSummary.xul
new revision: 1.4; previous revision: 1.3
done
Checking in p3pSummary.js;
/cvsroot/mozilla/extensions/p3p/resources/content/p3pSummary.js,v  <-- 
p3pSummary.js
new revision: 1.9; previous revision: 1.8
done