Opened 10 years ago

Last modified 6 years ago

#1434 reopened defect

Pressing Esc does not stop page load

Reported by: benzea Owned by: humitos
Priority: Low Milestone:
Component: Browse Version: Unspecified
Severity: Unspecified Keywords: patch
Cc: manuq, humitos, godiard Distribution/OS: Unspecified
Bug Status: Unconfirmed

Description

In Browse 104 I cannot press Escape to cancel the page load. Pressing the stop button works fine though.

Attachments (5)

0001-Escape-key-stops-page-load-SL-3373.patch (1.1 KB) - added by humitos 8 years ago.
0001-Escape-key-stops-page-loading-SL-3373.patch (2.6 KB) - added by humitos 7 years ago.
v2 - considers full screen mode
escape_toolkit.patch (755 bytes) - added by erikos 7 years ago.
add overwritable escape method
escape_browse.patch (2.2 KB) - added by erikos 7 years ago.
use overwrite method
webkit_load_status.py (1001 bytes) - added by humitos 7 years ago.
Test case for Esc and spinning cursor

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by tomeu

  • Component changed from sugar to Browse
  • Owner changed from tomeu to erikos

Looks like a missing accelerator.

comment:2 Changed 9 years ago by RafaelOrtiz

  • Milestone changed from Unspecified by Release Team to 0.94

Confirmed, moving to 0.94.

comment:3 Changed 7 years ago by manuq

  • Milestone changed from 0.94 to 0.98

Patch looks good, would be nice to ask for design approval, and if there's positive feedback, push it in the next cycle (0.98).

comment:4 follow-up: Changed 7 years ago by garycmartin

Just tested in Safari, Firefox, and Chrome, they all use the esc key to stop a page load so seems an OK additional keybinding to add to Browse. The only potential conflict in Sugar is that esc is used to exit from fullscreen mode in Activities that support it (e.g. Turtle Art, Image Viewer). If Browse adds a fullscreen feature as some point in the future, esc would need some extra logic. Something like 1) if in fullscreen mode and a page is not loading, exit fullscreen mode; 2) if in fullscreen mode and a page is loading, stop loading -- this is how Safari handles the esc case.

comment:5 follow-up: Changed 7 years ago by humitos

  • Cc manuq humitos godiard added
  • Keywords patch added

manuq or gonzalo, can you re-consider applying this patch?

comment:6 in reply to: ↑ 5 Changed 7 years ago by godiard

Replying to humitos:

manuq or gonzalo, can you re-consider applying this patch?

How you solve the conflict with the exit of fullscreen mode (see previous comment by ary)

comment:7 Changed 7 years ago by humitos

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

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

Replying to garycmartin:

Something like 1) if in fullscreen mode and a page is not loading, exit fullscreen mode; 2) if in fullscreen mode and a page is loading, stop loading -- this is how Safari handles the esc case.

Well, this is not so easy to do because in that case we have to modify the Activity class to handle this situation. In fact, I think it's not a good idea to modify the Activity class just for this special situation. Maybe we can think in something more general like defining a "state" (boolean) variable to add in the if clause.

At the moment, Sugar's Activity class take priority over the "key-press" event (sugar3/graphics/window.py L250). So, if we are loading a web page in fullscreen and we press Esc, we will be "unfullscreen" and the web page will keep loading.

comment:9 in reply to: ↑ 8 Changed 7 years ago by manuq

Replying to humitos:

Replying to garycmartin:

Something like 1) if in fullscreen mode and a page is not loading, exit fullscreen mode; 2) if in fullscreen mode and a page is loading, stop loading -- this is how Safari handles the esc case.

Well, this is not so easy to do because in that case we have to modify the Activity class to handle this situation. In fact, I think it's not a good idea to modify the Activity class just for this special situation. Maybe we can think in something more general like defining a "state" (boolean) variable to add in the if clause.

Remember classes and inheritance? :P You can redefine the callback (as we did in Terminal some months ago #440) Just remember to call the parent's callback. Is a bit hackish because is a private method and have the name mangling with the two underscores, but I think doable.

comment:10 Changed 7 years ago by humitos

  • Priority changed from Unspecified by Maintainer to Low

Changed 7 years ago by humitos

v2 - considers full screen mode

comment:11 Changed 7 years ago by manuq

  • Resolution set to fixed
  • Status changed from accepted to closed

Thanks! Pushed as 41968d61 .

Changed 7 years ago by erikos

add overwritable escape method

Changed 7 years ago by erikos

use overwrite method

comment:12 follow-up: Changed 7 years ago by erikos

  • Resolution fixed deleted
  • Status changed from closed to reopened

As said in http://bugs.sugarlabs.org/ticket/4013#comment:8 I am not happy with the approach of these hacks. I would do it more in the way as in the patches attached above .

However I wonder if we should handle that case. When I am in fullscreen mode and a page does load that I want to stop I do have to be in laptop mode to use the escape key, ebook mode will not work. Furthermore if the page did not stop I have an unloaded state. What can I do now? Reload the page? Go back to the previous one? Type in a new url? For all of those I have to exit fullscreen mode.

I really think this is an edge edge case and would vote for removing that code.

comment:13 in reply to: ↑ 12 Changed 7 years ago by erikos

Replying to erikos:

I really think this is an edge edge case and would vote for removing that code.

(The part about handling the escape key in fullscreen mode)

comment:14 Changed 7 years ago by erikos

Actually, the fullscreen mode in Browse is strange in the first place because we do not have the options of 'new url', 'reload' or 'back/forward'. At least in ebook mode without shortcuts. All this really sounds strange to me :) Do we have use cases for fullscreen Browse mode? Is that usable as is? Or just a show case?

/me is in question mode today

comment:15 Changed 7 years ago by manuq

+1 to erikos, I now think would be better that the first escape press exits fullscreen, and a second one stops loading, if a loading is being processed.

And if changing the toolkit is allowed for this, yes is a better option to do it that way.

comment:16 Changed 7 years ago by manuq

  • Keywords olpc-test-pending added
  • Resolution set to fixed
  • Status changed from reopened to closed

This was fixed by Simon and pushed as f9288d2a .

comment:17 follow-up: Changed 7 years ago by greenfeld

  • Keywords olpc-test-pending removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

We stop as expected but do not reset the busy cursor in Browse-145/OLPC 13.1.0 os9 in two cases:

  1. Enter a URL in the search bar and press Enter. Quickly press ESC to abort the load. Press the fullscreen button. You will now see the title of the page (if loaded) with a spinning busy cursor and gray main area (or partial page load). The busy cursor will continue spinning after you exit fullscreen mode.
  2. While in fullscreen mode and loading a page quickly press ESC twice to exit fullscreen mode and then abort loading the page. Sometimes the busy cursor keeps spinning in this case as well.

Changed 7 years ago by humitos

Test case for Esc and spinning cursor

comment:18 in reply to: ↑ 17 Changed 7 years ago by humitos

Replying to greenfeld:

We stop as expected but do not reset the busy cursor in Browse-145/OLPC 13.1.0 os9 in two cases:

I found the issue here and it has to do with the load-status property of WebKit.WebView.

The problem is that after a WEBKIT_LOAD_FAILED state it goes to WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT and that state is one of the ones that are of our interest to show the spinning cursor.

I'm not sure if that is a bug on WebKitGTK+ or not. I'm asking on #webkitgtk+

I attached a test case that works with my internet connection speed (it's difficult to catch the exact moment to stop the loading)

comment:19 Changed 7 years ago by humitos

I filled a new ticket on the WebKitGTK+ bugtracker to ask about this behaviour.

comment:20 Changed 6 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.