Closed Bug 1044960 Opened 11 years ago Closed 11 years ago

[dolphin][flame][Browser] window.open opens new tab in background and could not come to frontend automatically

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Browser
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 wontfix)

Status:
RESOLVED FIXED
Milestone:
2.1 S2 (15aug)
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- wontfix

People

(Reporter: defang.zhang, Assigned: jj.evelyn)

References

(
URL
)

Details

(Whiteboard: [sprd 335549])

Attachments

(2 files)

point to https://github.com/mozilla-b2g/gaia/pull/22656
46 bytes, text/x-github-pull-request
benfrancis
: review+
bajaj
: approval-gaia-v2.0+
Details | Review
bug-1044960-gecko-fix-malformed-json.patch
1.17 KB, patch
kats
: review+
bajaj
: approval-mozilla-b2g32+
Details | Diff | Splinter Review
DEFECT DESCRIPTION: web page keep in loading status when open test web page Steps to reproduce: ---------------------------------------------------- 1.open 116.228.149.59 website,select WAP Test->WAP Download Test->JavaScript Test->browser object->windows object->new windows focus 2.click botton of Open window Actual result: --------------------------------------- web page keep in loading status when click botton of Open window Expected result: --------------------------------------- display normal Additional info: -------------------------------------- Reproduce rate: 5/5
url: http://116.228.149.59/WAP/javascript/Browser_Object/windows_object/new_window_focus.htm source code <html> <head> <script type="text/javascript"> function openWin() { myWindow=window.open('','','width=200,height=100'); myWindow.document.write("<p>This is 'myWindow'</p>"); myWindow.focus(); } </script> </head> <body> <input type="button" value="Open window" onclick="openWin()" /> </body> </html>
Summary: [dolphin][Browser]web page keep in loading status when open test web page → [dolphin][flame][Browser]web page keep in loading status when open test web page
Flags: needinfo?(wchang)
Flags: needinfo?(ryang)
Flags: needinfo?(pcheng)
This script can open a new window on android and ios. But on ffos, browser does not support new window. So I think this might be a feature request. But anyway, we might want to show an error msg instead of keeping loading.
Flags: needinfo?(pcheng)
In fact, if you click some other place, such as '+' button then return. You will find the window of This is 'myWindow'. very strange.
Buri V1.3 and Flame v1.4 also has this problem. The behavior we saw is: after clicking the open window button, there is a new tab opened in background but the new tab was unable to be brought to frontend.
Summary: [dolphin][flame][Browser]web page keep in loading status when open test web page → [dolphin][flame][Browser] window.open opens new tab in background and could not come to frontend automatically
(In reply to Peipei Cheng from comment #4) > Buri V1.3 and Flame v1.4 also has this problem. > > The behavior we saw is: > after clicking the open window button, there is a new tab opened in > background but the new tab was unable to be brought to frontend. "the new tab was unable to be brought to frontend." means it didn't come to frontend automatically. User need to go to tab list and manually bring it to front end.
Evelyn, Can someone comment if this is meant to be supported on the 1.4 release? Thanks
Flags: needinfo?(wchang)
Flags: needinfo?(ryang)
Flags: needinfo?(ehung)
Whiteboard: [sprd 335549]
It's not a bug, neither a feature request. The testing page opens *nothing* so browser app shows an 'about:blank' page which is the landing page of browser. The behavior makes sense to me although the UI is a bit weird (displaying a progress bar). I don't think we need to care this test case, because nobody will write a web page like this. BTW, the first parameter of |window.open| is not optional, and use |document.write| to replace content of a page is not a good practice. https://developer.mozilla.org/en-US/docs/Web/API/Window.open
Flags: needinfo?(ehung)
(In reply to Evelyn Hung [:evelyn] from comment #7) > It's not a bug, neither a feature request. The testing page opens *nothing* > so browser app shows an 'about:blank' page which is the landing page of > browser. The behavior makes sense to me although the UI is a bit weird > (displaying a progress bar). I don't think we need to care this test case, > because nobody will write a web page like this. > > BTW, the first parameter of |window.open| is not optional, and use > |document.write| to replace content of a page is not a good practice. > > https://developer.mozilla.org/en-US/docs/Web/API/Window.open Dear Evelyn: As you say,the PC should same phenomenon.In fact,the same test code is normal display in PC use firefox browser.please tell me why? Thanks!
So I found a gecko error: E/GeckoConsole( 2673): [JavaScript Error: "JSON.parse: unexpected character at line 1 column 165 of the JSON data"] E/GeckoConsole( 2673): [JavaScript Error: "metrics is null" {file: "chrome://global/content/BrowserElementPanning.js" line: 502}] The broken JSON looks like this: { "x" : 0, "y" : 0, "viewport" : { "width" : 0, "height" : 0 }, "cssPageRect" : { "x" : 0, "y" : 0, "width" : 320, "height" : 489.333 }, "resolution" : { "width" : Infinity }, "cssCompositedRect" : { "width" : 320, "height" : 489.333 } } Note the resolution.width is Infinity, not a valid value in JSON. The value is calculated by gfx/layers/FrameMetrics.h#193 CSSToScreenScale CalculateIntrinsicScale() const { return CSSToScreenScale( std::max(mCompositionBounds.width / mViewport.width, mCompositionBounds.height / mViewport.height)); } and it happens to be a divide-by-zero error.
Oh, after fixing the broken JSON, I still can see the issue - Browser app doesn't refresh the page.
This is a special case handling: window.open('') a empty string url, and do document.write to trigger a locationchange event. The content of the new created tab will be hidden by start screen, so in my patch, I hide start screen in this case.
Attachment #8469828 - Flags: review?(bfrancis)
This patch fix the JSON error in comment 9. Instead of fixing the divide-by-zero error (which is risky because I don't know why the mViewport.width and mViewport.height are zero), I found these four lines are useless. They were introduced in bug 985817 which did a bit code refactoring but adding these lines with doubt. He's guess is true - the 'Viewport:Change' message receiver doesn't use the resolution.width value. http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#44 So I think it's safe to remove it.
Attachment #8469830 - Flags: review?(bugmail.mozilla)
Assignee: ttsai → ehung
Comment on attachment 8469830 [details] [diff] [review] bug-1044960-gecko-fix-malformed-json.patch Review of attachment 8469830 [details] [diff] [review]: ----------------------------------------------------------------- The change looks fine. Next time please make sure you have author info and a proper commit message in the patch as well.
Attachment #8469830 - Flags: review?(bugmail.mozilla) → review+
Apologies for late flag setting, this is required for 1.4 partner. Thanks Evelyn and Kats!
blocking-b2g: --- → 1.4+
Keywords: checkin-needed
(In reply to Evelyn Hung [:evelyn] from comment #12) > Created attachment 8469830 [details] [diff] [review] > bug-1044960-gecko-fix-malformed-json.patch > > This patch fix the JSON error in comment 9. Instead of fixing the > divide-by-zero error (which is risky because I don't know why the > mViewport.width and mViewport.height are zero), I found these four lines are > useless. They were introduced in bug 985817 which did a bit code refactoring > but adding these lines with doubt. He's guess is true - the > 'Viewport:Change' message receiver doesn't use the resolution.width value. > > http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/ > BrowserElementPanning.js#44 > > So I think it's safe to remove it. Dear Evelyn: This modify of yours is exist in master branch,but in mozillaorg/v1.4 branch not found your modify content.Please help check.Thanks!
(In reply to defang.zhang from comment #15) > Dear Evelyn: > This modify of yours is exist in master branch,but in mozillaorg/v1.4 branch > not found your modify content.Please help check.Thanks! Bug 985817 landed on mozilla31, which was tracking B2G 2.0. Presumably that means the Gecko fix isn't needed on v1.4.
(In reply to Wayne Chang [:wchang] from comment #14) > Apologies for late flag setting, this is required for 1.4 partner. > > Thanks Evelyn and Kats! I think the checkin-needed flag you set was premature. The gaia patch doesn't appear to have been reviewed yet.
Also, the PR was against v1.4 when standard branch landing practice dictates that patches land on master first. Please submit a PR against master, get review, then request checkin. Gaia patch reverted from v1.4: v1.4: https://github.com/mozilla-b2g/gaia/commits/v1.4
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/804f861790a0 Assuming the lines are equally-useless for v2.0, you should request b2g32 approval on this patch.
Comment on attachment 8469828 [details] [review] point to https://github.com/mozilla-b2g/gaia/pull/22656 This is a bit of a weird edge case, but r+ from me. This should probably be re-based against master and landed there first then uplifted.
Attachment #8469828 - Flags: review?(bfrancis) → review+
Flags: in-moztrap?(smiko)
Test case on this issue: https://moztrap.mozilla.org/manage/case/4073/
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?]
(In reply to Ben Francis [:benfrancis] from comment #21) > Comment on attachment 8469828 [details] [review] > point to https://github.com/mozilla-b2g/gaia/pull/22656 > > This is a bit of a weird edge case, but r+ from me. This should probably be > re-based against master and landed there first then uplifted. I sent a PR against 1.4 because it's said that browser app on master should not have any change... We can land this PR for 1.4, and I send another one (or cherry pick this) to master. Thanks for the review. :)
Given it's 1.4+, we won't land it back to 1.3t. clear 1.3T affected flag.
status-b2g-v1.3T: affected → ---
(In reply to Evelyn Hung [:evelyn] from comment #23) > (In reply to Ben Francis [:benfrancis] from comment #21) > > Comment on attachment 8469828 [details] [review] > > point to https://github.com/mozilla-b2g/gaia/pull/22656 > > > > This is a bit of a weird edge case, but r+ from me. This should probably be > > re-based against master and landed there first then uplifted. > > I sent a PR against 1.4 because it's said that browser app on master should > not have any change... We can land this PR for 1.4, and I send another one > (or cherry pick this) to master. > Ooops, just realized what happened in comment 16 to 20. Okay, I will send a PR for master first.
(In reply to Evelyn Hung [:evelyn] from comment #25) > (In reply to Evelyn Hung [:evelyn] from comment #23) > Ooops, just realized what happened in comment 16 to 20. Okay, I will send a > PR for master first. I decided to land my patch in v1.4 only, because 1. on master we are migrating to system browser, and browser app has been deprecated. 2. the code of system browser is different from current browser app, and the symptom is different. So I filed bug 1054225 for master. Hi Ryan, For the gaia patch, is it okay to pick my first PR back to 1.4? For the gecko patch, I think only landed to m-c is okay because 1.4 is not affected. Thanks!
Flags: needinfo?(ryanvm)
Sounds good. Is this all wontfix for v2.0? v1.4: https://github.com/mozilla-b2g/gaia/commit/93b469e3643f6caed8e190c72b612c60f6a74a1e
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: --- → ?
status-b2g-v2.1: --- → wontfix
Flags: needinfo?(ryanvm) → needinfo?(ehung)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
Comment on attachment 8469830 [details] [diff] [review] bug-1044960-gecko-fix-malformed-json.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 985817 User impact if declined: a web page bumping into this special case will fail to be opened in browser app. Testing completed: manually test on m-c/master Risk to taking this patch (and alternatives if risky): no String or UUID changes made by this patch: no
Attachment #8469830 - Flags: approval-mozilla-b2g32?
Flags: needinfo?(ehung)
Comment on attachment 8469828 [details] [review] point to https://github.com/mozilla-b2g/gaia/pull/22656 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] [Bug caused by] (feature/regressing bug #): n/a [User impact] if declined: a web page bumping into this special case will fail to be opened in browser app. [Testing completed]: manually test on m-c/master [Risk to taking this patch] (and alternatives if risky): no [String changes made]: no
Attachment #8469828 - Flags: approval-gaia-v2.0?
Test case added in moztrap: https://moztrap.mozilla.org/manage/case/14345/
URL: https://moztrap.mozilla.org/manage/ca...
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(smiko)
Flags: in-moztrap+
Hi, I'm not sure whom to ask 2.0 approval, but my approval-gaia-v2.0? has already there for a couple of weeks. Could you help? Thanks!
Flags: needinfo?(bbajaj)
Attachment #8469828 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Flags: needinfo?(bbajaj)
Attachment #8469830 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Ryan, I got approval. Thanks for your help. :)
status-b2g-v2.0: ? → affected
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: