551711 - Open Link in New Tab via content panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

Support opening links in new tabs via a context panel. Long taps on a link would display a context panel with the command to open the link in a new tab.

Comment 1

[Aakash Desai [:aakashd]]

Basic mock: http://www.flickr.com/photos/madhava_work/4404456833/

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 2

This patch has:
* Support for context panel in web content area
* Context commands are <richlistitem>s in the context-command list, which supports panning
* Each command has a "type", which can be "image", "link" or both
* When opened, the context panel will filter the commands based on type. If no commands are applicable, the panel is not opened.
* The URL for the link (fallback) or image is displayed in the panel header
* Support for Open in New Tab and Save Image (it's was free) is included
* Mimics the desktop contextmenu properties for popupNode, onLink, linkURL, onImage and mediaURL.

Still need to:
* Make prettier
* Use link, image and link+image icon
* Support background images

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 432831 [details] [diff] [review]
WIP 2



>+    let label = document.getElementById("context-hint");
>+    if (this.onImage)
>+      label.value = this.mediaURL;
>+    if (this.onLink)
>+      label.value = this.linkURL;

maybe you could put those into the above if/else?

>
>+
>+    container.top = (window.innerHeight - height) / 2;
>+    container.left = (window.innerWidth - width) / 2;

Positionning seems to act randomly for me when i start to approch the edge of the viewport.


Also, I wondering if we should add a little "something" around the link/image when the context menu is show to be sure to remember which link I'm going to click on. (maybe the title area of the content panel is done for that).

A little bug because we don't check the target of the event I guess: see attached screenshot

Overall it looks good!

I know paul hardly wants such a feature for a while and have put that into at the top of his priority list!

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug screenshot

Screenshot of the bug described in the previous comment

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Error: this.popupNode is null
Source File: chrome://browser/content/browser-ui.js
Line: 1702

I'm not sure when this happen.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 3

* Fixes Vivien's bug
* Moves strings to DTD
* Supports background images

Needs some style tweaks before final review

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch should be ready to go. It fixes a problem with clearing the click buffer in InputHandler and it styles the panel close to the proposed look.

It uses a bit of a hack to set the first-child and last-child for CSS styling purposes.

I was considering changing the code to give InputHandler a way to call a contextMenu(...) method on the clicker interface, but skipped it for now.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

This version also removes support for background images. We ended up thinking they added too much noise to the menu.

Screenshot:
http://www.flickr.com/photos/finkle/4457321089/

(we removed the image from the upper left corner too)

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 434338 [details] [diff] [review]
patch

> MouseModule.prototype = {
>   handleEvent: function handleEvent(evInfo) {
>+    if (evInfo.event.type == "contextmenu")
>+      this._cleanClickBuffer();
>+

This is not the most elegant way to do it, but I guess this is ok until we create a real ContextModule. 
Could we create a followup for that and add a comment in the patch?

>+
>+  _getComputedURL: function ch_getComputedURL(aElem, aProp) {
>+    let url = aElem.ownerDocument.defaultView.getComputedStyle(aElem, "").getPropertyCSSValue(aProp);
>+    if (url instanceof CSSValueList) {
>+      if (url.length != 1)
>+        throw "found multiple URLs";
>+      url = url[0];
>+    }
>+    return url.primitiveType == CSSPrimitiveValue.CSS_URI ? url.getStringValue() : null;
>+  },

Without the background support you probably don't need this function anymore

>+  handleEvent: function ch_handleEvent(aEvent) {
>+    this._clearState();
>+
>+    let [elementX, elementY] = Browser.transformClientToBrowser(aEvent.clientX, aEvent.clientY);
>+    this.popupNode = Browser.elementFromPoint(elementX, elementY);
>+
>+    // Do checks for nodes that never have children.
>+    if (this.popupNode.nodeType == Node.ELEMENT_NODE) {
>+      // See if the user clicked on an image.
>+      if (this.popupNode instanceof Ci.nsIImageLoadingContent && this.popupNode.currentURI) {
>+        this.onImage = true;
>+        this.mediaURL = this.popupNode.currentURI.spec;
>+      }
>+    }
>+
>+    let elem = this.popupNode;
>+    while (elem) {
>+      if (elem.nodeType == Node.ELEMENT_NODE) {
>+        // Link?
>+        if (!this.onLink &&
>+             ((elem instanceof HTMLAnchorElement && elem.href) ||
>+              (elem instanceof HTMLAreaElement && elem.href) ||
>+              elem instanceof HTMLLinkElement ||
>+              elem.getAttributeNS("http://www.w3.org/1999/xlink", "type") == "simple")) {
>+          
  
Use a const for the namespace


>+    let first = last = null;
>+    let commands = document.getElementById("context-commands");
>+    for (let i=0; i<commands.childElementCount; i++) {

I wonder if accessing childElementCount is faster than childNodes.length (I guess that the response is yes)
Maybe we should do that everywhere

>+      let command = commands.children[i];
>+      let type = command.getAttribute("type");
>+      command.removeAttribute("selector");
>+      if (type.indexOf("image") != -1 && this.onImage) {
>+        first = (first ? first : command);
>+        last = command;
>+        command.hidden = false;
>+        hasCommand = true;

hasCommand is unused.

>+    let rect = container.getBoundingClientRect();
>+    let height = rect.height;
>+    if (height > 0.75 * window.innerHeight) {
>+      height = 0.75 * window.innerHeight;
>+      container.height = height;
>+    }

container.height = Math.min(height, 0.75 * window.innerHeight);

>+
>+    let width = rect.width;
>+    if (width > 0.75 * window.innerWidth) {
>+      width = 0.75 * window.innerWidth;
>+      container.width = width;
>+    }

container.width = Math.min(width, 0.75 * window.innerWidth);

Maybe you're trying to avoid changing the height/width each time you show the context menu?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> (From update of attachment 434338 [details] [diff] [review])
> > MouseModule.prototype = {
> >   handleEvent: function handleEvent(evInfo) {
> >+    if (evInfo.event.type == "contextmenu")
> >+      this._cleanClickBuffer();
> >+
> 
> This is not the most elegant way to do it, but I guess this is ok until we
> create a real ContextModule. 
> Could we create a followup for that and add a comment in the patch?

filed bug 554639

> >+  _getComputedURL: function ch_getComputedURL(aElem, aProp) {

> >+  },
> 
> Without the background support you probably don't need this function anymore

Removed

> >+  handleEvent: function ch_handleEvent(aEvent) {

> >+              elem.getAttributeNS("http://www.w3.org/1999/xlink", "type") == "simple")) {
> 
> Use a const for the namespace

Done

> >+    let first = last = null;
> >+    let commands = document.getElementById("context-commands");
> >+    for (let i=0; i<commands.childElementCount; i++) {
> 
> I wonder if accessing childElementCount is faster than childNodes.length (I
> guess that the response is yes)
> Maybe we should do that everywhere

Maybe, but I wanted to make sure I didn't get any non-elements

> >+      let command = commands.children[i];
> >+      let type = command.getAttribute("type");
> >+      command.removeAttribute("selector");
> >+      if (type.indexOf("image") != -1 && this.onImage) {
> >+        first = (first ? first : command);
> >+        last = command;
> >+        command.hidden = false;
> >+        hasCommand = true;
> 
> hasCommand is unused.

Removed

> container.height = Math.min(height, 0.75 * window.innerHeight);

> container.width = Math.min(width, 0.75 * window.innerWidth);

> Maybe you're trying to avoid changing the height/width each time you show the
> context menu?

Nah, your code is better.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 434535 [details] [diff] [review]
patch 2

Updated patch

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 434535 [details] [diff] [review]
patch 2

> MouseModule.prototype = {
>   handleEvent: function handleEvent(evInfo) {
>+    if (evInfo.event.type == "contextmenu")
>+      this._cleanClickBuffer();
>+

Maybe a quick comment to indicate the bug number (554639) will be nice.


Welcome context panel!

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with TODO comment:
http://hg.mozilla.org/mobile-browser/rev/c37ed87d7a3a

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100325 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a3pre) Gecko/20100325 Namoroka/3.7a3pre Fennec/1.1a2pre


follow-up bugs:
mfinkle filed https://bugzilla.mozilla.org/show_bug.cgi?id=554951

Comment 16

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11654 has been created to regression test this bug.

Comment 17

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11741 created for l10n string guide