Closed Bug 551711 Opened 10 years ago Closed 10 years ago

Open Link in New Tab via content panel

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attached patch WIP 2 (obsolete) — Splinter Review
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
Assignee: nobody → mark.finkle
Attachment #431893 - Attachment is obsolete: true
Attachment #432831 - Flags: feedback?(21)
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!
Attachment #432831 - Flags: feedback?(21) → feedback+
Attached image bug screenshot
Screenshot of the bug described in the previous comment
Error: this.popupNode is null
Source File: chrome://browser/content/browser-ui.js
Line: 1702

I'm not sure when this happen.
Attached patch WIP 3 (obsolete) — Splinter Review
* Fixes Vivien's bug
* Moves strings to DTD
* Supports background images

Needs some style tweaks before final review
Attachment #432831 - Attachment is obsolete: true
Summary: Open in New Tab via content panel → Open Link in New Tab via content panel
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #432888 - Attachment is obsolete: true
Attachment #434338 - Flags: review?(21)
Attachment #434338 - Flags: feedback?(webapps)
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 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?
(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.
Attached patch patch 2Splinter Review
Attachment #434535 - Flags: review?
Comment on attachment 434535 [details] [diff] [review]
patch 2

Updated patch
Attachment #434535 - Flags: review? → review?(21)
Attachment #434338 - Attachment is obsolete: true
Attachment #434338 - Flags: feedback?(webapps)
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!
Attachment #434535 - Flags: review?(21) → review+
pushed with TODO comment:
http://hg.mozilla.org/mobile-browser/rev/c37ed87d7a3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11654 has been created to regression test this bug.
Flags: in-litmus? → in-litmus+
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11741 created for l10n string guide
You need to log in before you can comment on or make changes to this bug.