Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4236 closed defect (fixed)

Issue with popup window

Reported by: erikos Owned by: humitos
Priority: Normal Milestone:
Component: Browse Version: 0.97.x
Severity: Major Keywords: r+, olpc-test-passed
Cc: erikos, dnarvaez Distribution/OS: Unspecified
Bug Status: Assigned

Description

Page: http://www.dhl.de/en/paket/kundenservice/sendungsverfolgung.html

Description: This page does open a new page to show the search result.

Result: does not open on the XO, does work in Epiphany on the XO.

Test cases for wanted/unwanted popup windows:

http://dev.laptop.org/~erikos/wanted.html

http://dev.laptop.org/~erikos/unwanted.html

Attachments (3)

target_blank.html (400 bytes) - added by humitos 8 years ago.
Testcase with target="_blank" on a <a> and <form>
0001-Handle-create-web-view-signal-and-open-a-new-tab-SL-.patch (3.7 KB) - added by humitos 8 years ago.
Patch that handles 'create-web-view'
0001-Handle-create-web-view-signal-and-open-a-new-tab-SL-.2.patch (4.3 KB) - added by humitos 8 years ago.
v2 - this patch removes the hanlding of the signal 'new-window-policy-decision-requested' and handle 'web-view-ready' to be sure that we can show the WebView

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by dnarvaez

This works as expected for me in sugar-build, both F17 and F18.

comment:2 Changed 8 years ago by dnarvaez

Actually this works fine for me on the XO too, build 13.

comment:3 Changed 8 years ago by erikos

Hi Daniel, if you scroll down a bit on the page http://www.dhl.de/en/paket/kundenservice/sendungsverfolgung.html there is a field track&trace. If you enter the code '350043001668' there and click 'Search now' nothing happens. And that result would open in a new tab.

comment:4 Changed 8 years ago by dnarvaez

Yeah, sorry the testcases works but not the dhl site.

I had a look at the code and I think our popups implementation is wrong. We need to implement

http://webkitgtk.org/reference/webkitgtk/stable/webkitgtk-webkitwebview.html#WebKitWebView-create-web-view

(verified that's actually called on dhl)

And I suspect we should only be making policy decisions in new-window-policy-decision-requested.

comment:6 follow-up: Changed 8 years ago by erikos

The two examples I added above http://dev.laptop.org/~erikos/wanted.html and http://dev.laptop.org/~erikos/unwanted.html does work as expected on the XO. The former does open the new window in a new tab if I click on the button. The latter does block the popup window.

Now, the question is, why the packet example (dhl.de) does work in Epiphany but not in Browse.

I checked as well that the popup page itself can be rendered fine in Browse. If you put in the tracking id in the page http://nolp.dhl.de/nextt-online-public/set_identcodes.do you get to the page fine and to the information. So the issue must really be the popup window part.

comment:7 Changed 8 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Keywords triage removed
  • Milestone changed from Unspecified by Release Team to 0.98
  • Priority changed from Unspecified by Maintainer to Normal
  • Severity changed from Unspecified to Major
  • Version changed from Unspecified to 0.97.x

Thanks for the pointers Daniel, so we are know now where we have to adjust. Let's move this into 0.98.

comment:8 Changed 8 years ago by humitos

  • Owner changed from manuq to humitos
  • Status changed from new to accepted

comment:9 in reply to: ↑ 6 Changed 8 years ago by humitos

  • Cc erikos dnarvaez added
  • Keywords r? added

Replying to erikos:

The two examples I added above http://dev.laptop.org/~erikos/wanted.html and http://dev.laptop.org/~erikos/unwanted.html does work as expected on the XO. The former does open the new window in a new tab if I click on the button. The latter does block the popup window.

Those examples use Javascript code (window.open(url)) to create a new window with the URL given. I understand that case is already managed by the signal: new-window-policy-decision-requested

Now, the question is, why the packet example (dhl.de) does work in Epiphany but not in Browse.

The site you mentioned here uses target="_blank" on its form and that is handled by(*): 'create-web-view' signal. That is the signal we are not handling.

I've got a patch that I'm testing to be sure that it cover these cases.

(*) I can't confirm that because I've just create an index.html to test that and it worked on build 13. I don't understand what is the difference between 'new-window-policy-decision-requested' and 'create-web-view'. Another reason could be some strange javascript connected to the submit button that I'm not seeing.

Anyway, I'm uploading the patch that I have here tested on many cases to see if someone else find another case that it doesn't work or to take a look what I did.

Changed 8 years ago by humitos

Testcase with target="_blank" on a <a> and <form>

Changed 8 years ago by humitos

Patch that handles 'create-web-view'

comment:10 follow-up: Changed 8 years ago by dnarvaez

Couple of comments.

According to the API docs you should not display the window in create-web-view, you should wait until web-view-ready. It's not clear if that's just to have the features information available, but in doubt I'd follow the docs literally. Web pages do all kinds of weird stuff with popups and it's easy to overlook bugs in testing.

I haven't tested this but I would guess create-web-view will work for all kind of popups. So I think the current new-window-policy-decision-requested code can/should be removed. It doesn't seem right anyway.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by humitos

Replying to dnarvaez:

According to the API docs you should not display the window in create-web-view, you should wait until web-view-ready. It's not clear if that's just to have the features information available

Yes, I read the docs and this is true. I didn't use that because we don't have "windows" inside Browse and our tabs doesn't have size / position / status and scrollbars properties. So, I think it's no needed.

Do you agree with me?

I haven't tested this but I would guess create-web-view will work for all kind of popups. So I think the current new-window-policy-decision-requested code can/should be removed.

I will try this and I will let you know.

Thanks for your comments.

comment:12 in reply to: ↑ 11 Changed 8 years ago by dnarvaez

Replying to humitos:

Replying to dnarvaez:

According to the API docs you should not display the window in create-web-view, you should wait until web-view-ready. It's not clear if that's just to have the features information available

Yes, I read the docs and this is true. I didn't use that because we don't have "windows" inside Browse and our tabs doesn't have size / position / status and scrollbars properties. So, I think it's no needed.

Do you agree with me?

The problem is, I'm not sure without looking at the webkit code that the only reason they suggest to run it later is those features. IMO it would be safer to just show it later... Unless it's really hard to code or something.

Changed 8 years ago by humitos

v2 - this patch removes the hanlding of the signal 'new-window-policy-decision-requested' and handle 'web-view-ready' to be sure that we can show the WebView

comment:13 Changed 8 years ago by humitos

dnarvaez: I've just attached a new version of this patch that removes the 'new-window-policy-decision-requested' and makes use of 'web-view-ready'

comment:14 Changed 8 years ago by erikos

Thanks Humitos your patch passes the following cases:

  • the dhl packet page

If Daniel agrees with the patch itself, I am ok and we can push it. Looks good to me at first glance.

comment:15 Changed 8 years ago by dnarvaez

I have not tested but patch looks good to me in theory!

comment:16 Changed 8 years ago by manuq

  • Keywords r+ olpc-test-pending added; r? removed
  • Resolution set to fixed
  • Status changed from accepted to closed

Good work! Pushed b3d2fa7d .

comment:17 Changed 8 years ago by greenfeld

  • Keywords olpc-test-passed added; olpc-test-pending removed

Browse-148 passes these popup testcases in OLPC 13.1.0 os20.

comment:18 Changed 8 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.