1021385 - Sheets opened as a link in a new tab have "undefined" name

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Valentin Tsatskin [:vt]]

Attachment: 2014-06-05-11-42-16.png

# Build Information

Device: Flame
Gaia      d2cfef555dabab415085e548ed44c48a99be5c32
Gecko     https://hg.mozilla.org/mozilla-central/rev/51b428be6213
BuildID   20140605040202
Version   32.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

# Description

Sheets opened with "open link in new tab" from e.me results have sheet name as "undefined"

# Steps to Reproduce

1) Open an e.me result which is a web page.
2) Find a link and long press it
3) Press "open link in new tab"
4) Long press home for task manager

# Expected Results

Newly created sheet has proper name

# Actual Results

Newly created sheet has undefined name

# Reproduction Frequency: 

Every time

Comment 1

[Jason Smith [:jsmith]]

Nice find. Pretty sure this is a regression, but adding qawanted to see if this reproduces on 1.4.

Comment 2

[Cody Roesch [:croesch]]

Testing 1.4, I was NOT able to reproduce this bug because in E.me, long-tapping a link does not bring up the same window to allow a new page to be opened in a tab. I then tried with Browser and instead of seeing Undefined, I see Browser.

Environmental Variables:
Device: Flame 1.4
Build ID: 20140609000201
Gaia: 8b239e41bbd85aa7b6a2c5d388e775ba7de6fb2b
Gecko: e3f85877db29
Version: 30.0 (1.4) 
Firmware Version: v10G-2

Comment 3

[Ben Francis [:benfrancis]]

I'm pretty sure this feature is part of system browser and should not be turned on in 2.0.

Dale, I'm not sure what spec you were working from when you implemented this feature in bug 941266, but the spec I have says "Open link in a new window" with a + icon next to it rather than "Open link in new tab" which doesn't really make sense in this context as it actually opens a new window.

I don't think we should ship this feature without the accompanying changes to the task manager which make it a replacement for tab management.

What do you think about disabling this in the 2.0 branch?

Comment 4

[Dale Harvey (:daleharvey)]

The context menu should be enabled in all contexts (even apps), its more of a bug that it isnt than a new feature, we cant enable it for apps right now as too many of them dont handle events correctly but we should keep it on for the wrapper.

the undefined origin issue is one that we need to fix in a lot of places so it will be good to front load those fixes, cant remember which spec had the string, window does make more sense.

Comment 5

[Ben Francis [:benfrancis]]

I think that's a UX call and I've not seen it in any UX specs applicable to 2.0.

Francis, can you comment on whether we should have the context menu for all wrapper windows in 2.0, including an "open in new window" option which can create a new wrapper window from any link?

Previous to this patch wrapper windows could only be created by clicking on an icon in e.me results or a homescreen bookmark. They're treated very much like app windows which can only be triggered by clicking an icon. It seems odd to now be able to create arbitrary windows from any link like in a browser but without the full system browser experience. I think that's a UX call though.

Comment 6

[Dale Harvey (:daleharvey)]

It doesnt need to and probably shouldnt open in a wrapper window, we can easily switch it to open in a browser window.

Comment 7

[Pi Wei Cheng [:piwei] (inactive)]

We are missing builds on 5/18 in b2g-inbound for both Buri and Flame. Buri Mozilla inbound window doesn't give me a smaller pushlog either. So here is the best pushlog we could get.

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Buri
Build ID: 20140517084930
Gaia: d364c473012b7731688fe0ef4bca92358cee18c5
Gecko: f913ad39fa23
Version: 32.0a1
Firmware Version: v1.2device.cfg

First Broken Environmental Variables:
Device: Buri
BuildID: 20140519031732
Gaia: 2c45cebc622141d7e2726dc9fb09eeaa388ac7b2
Gecko: 3314ca6fc2a5
Version: 32.0a1
Firmware Version: v1.2device.cfg

Last Working Gaia / First Broken Gecko: Issue Does NOT reproduce
Gaia: d364c473012b7731688fe0ef4bca92358cee18c5
Gecko: 3314ca6fc2a5

Last Working Gecko / First Broken Gaia: Issue DOES reproduce
Gaia: 2c45cebc622141d7e2726dc9fb09eeaa388ac7b2
Gecko: f913ad39fa23

Gaia Pushlog:
https://github.com/mozilla-b2g/gaia/compare/d364c473012b7731688fe0ef4bca92358cee18c5...2c45cebc622141d7e2726dc9fb09eeaa388ac7b2

Comment 8

[Jason Smith [:jsmith]]

Broken by bug 941266.

Comment 9

[Francis Djabri [:djabber]]

(In reply to Ben Francis [:benfrancis] from comment #5)
> I think that's a UX call and I've not seen it in any UX specs applicable to
> 2.0.
> 
> Francis, can you comment on whether we should have the context menu for all
> wrapper windows in 2.0, including an "open in new window" option which can
> create a new wrapper window from any link?
> 
> Previous to this patch wrapper windows could only be created by clicking on
> an icon in e.me results or a homescreen bookmark. They're treated very much
> like app windows which can only be triggered by clicking an icon. It seems
> odd to now be able to create arbitrary windows from any link like in a
> browser but without the full system browser experience. I think that's a UX
> call though.

I see no harm in providing a context menu to allow users to open links, provided that it's in the browser, rather than in a new wrapper window.

Comment 10

[Dale Harvey (:daleharvey)]

The context menu was done before we switched to using activities for new windows, so a quick fix

Should the label be "open in new window" or "open in new tab", bearing in mind this will show on 2.0 and 2.1, in 2.0 it will open as a new tab in the browser, in 2.1 it will open in a wrapper window

Comment 11

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21055

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8446567 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21055

Looks good, need to keep a mental note when we disable the browser that this does the right thing, but while we have the browser it works

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ben, what's preventing this bug to land?

Comment 14

[Ben Francis [:benfrancis]]

Not sure, looks like Travis was red while I was away on PTO.

Rebased and landed in master https://github.com/mozilla-b2g/gaia/commit/0de0528329f02f4e4208481cc651e4c52930f3f2

Needs uplift to 2.0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/ab553918ead46f690d18c4559950436e319ff88c