905563 - [Buri][Browser]The browser should supply "open link in new tab" button when long tap a new

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect, P2)

Description

[sync-1]

Mozilla build ID:20130806071254
 
 Created an attachment (id=473164)
 logcat
 
 DEFECT DESCRIPTION:
 
  The browser should  supply  "open link in new tab" button when long tap a new
 
  REPRODUCING PROCEDURES:
 
  1.Open Browser app
 
  2.Open a web page : news.google.com
 
  3.long tap a news-->MS can't to bring up the "open link in new tab" menu-->KO
 
  EXPECTED BEHAVIOUR:
  
  The MS should to bing up the "open link in new tab" menu
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
  mid
  REPRODUCING RATE:
  5/5
  For FT PR, Please list reference mobile's behavior:
 
  Firefox browser in Android behavior：browser will to bing up the "open link in new tab" menu when long tap a news

Comment 1

[sync-1]

Clone from brother

Comment 2

[sync-1]

Attachment: logcat

Comment 3

[Ben Francis [:benfrancis]]

This only seems to happen on this page and may be caused by JavaScript on the page, but marking as sprintready to look into it.

Comment 4

[Viga]

What does sprintready mean?  Ready to fix on next release?

Comment 5

[Karen Rudnitski [:kar]]

I've been trying on a couple pages and can't seem to action with a long press (on m.bbc.co.uk/news for instance in trying to open an article in a new tab)

Comment 6

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55557746

Comment 7

[Karen Rudnitski [:kar]]

I've been trying on a couple pages and can't seem to action with a long press (on m.bbc.co.uk/news for instance in trying to open an article in a new tab)

Comment 8

[Viga]

news.google.com for instance.

Comment 9

[Viga]

Use firebug extension, I find the HTML of news.google.com  not using <a> ....</a>
as a link. So, our browser doesn't recognise the link.(can't open in a new tab)

The same problem while we save a picture on the web. When long press a picture shown on the 
web, which HTML isn't IMG taged...

I think we should add more to know they are actually a picture. Just like Fx for Android does.

BTW, I see same problem on master branch.

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi Dale,

I notice that if I long press a <a><span>link</span></a>, the nodeName would be |a| instead of |A|. I'm not quite sure if it's expected or not.

And one more thing, it's also fix a small regression of bug 907056, I just realize d whenever I long-press the content of browser, it will trigger mozbrowsercontextmenu event, so I add a line to set contextMenuHasCalled to false if no menuitem is created.

Comment 11

[Jack Liu]

Dears, is it possible to implement this feature in v1.1?

Comment 12

[Dale Harvey (:daleharvey)]

This feature is available in 1.1, this is fixing a relative edge case, I dont think we have control over leo flags at this point but I would expect this patch almost certainly wont be uplifted to 1.1 which has been long closed afaik

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 799308 [details]
PR to master

Hey George, cheers for this, good catch

So nodeName in html is supposed to be uppercase, either way 'A' is expected so instead of testing for 'a', we should just uppercase the nodeName prior to the check so 'a' and 'A' both work (https://developer.mozilla.org/en-US/docs/Web/API/element.tagName?redirectlocale=en-US&redirectslug=DOM%2Felement.tagName)

Also especially as we managed to introduce a regression, we are starting to require integration tests for features and bug fixes where it makes sense (particularly regressions), so clearing the review on this and if you could add tests that would be great. 

For this test we will want a new contextmenu_test.js, you can use https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/test/marionette/navigate_test.js as an example of how to load a new page, we can start with a very simple load a page, fire a context menu, ensure context menu fires callback to start with, if you hit any problems with the integration tests just ping on here / irc

Cheers
Dale

Comment 14

[Preeti Raghunath(:Preeti)]

QA,

Please check if this is reproducible on 1.0.1.

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 799308 [details]
PR to master

Hi Dale,

Test updated for 'open new page' case. Please kindly check.

Comment 16

[dkumar]

In response to comment 14

The issue does reproduce on v1.0.1,long pressing any article on news.google.com does not give the user option to open the article in new tab,whereas long pressing on any article on m.bbc.co.uk gives user the option to open the link in new tab.

Environmental Variables
Build ID: 20130906043205
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 054cdc27404e2daca91d3065d9783681032b2151
Platform Version: 18.0

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 799308 [details]
PR to master

Great thanks

Comment 18

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/083c8410a44b52699b3cff4065f653bd38033e4c

Comment 19

[Preeti Raghunath(:Preeti)]

New feature - will not take in 1.1