[BUG]: anchors break when a page includes '<meta http-equiv="Content-Type" ...'
If you navigate to a page with a '<meta http-equiv="Content-Type" ...' in the head element then any anchor in the URL is ignored. (This breaks my local cinema's web page because it puts listings and film descriptions on the same page and links them with anchors.) I think the reason is that the meta element forces the page to be reloaded by Nav_push() and this does not call a_Nav_expect_done(). Unfortunately a_Nav_expect_done() is responsible for calling a_UIcmd_set_scroll_by_fragment(), so any page pulled in by Nav_push() does not scroll to the anchor. Any ideas how to fix this? I having a hard time getting my head around this bit of the code. Regards, Jeremy Henty
On Sun, Mar 23, 2008 at 11:37:35PM +0000, Jeremy Henty (me) wrote:
I think the reason is that the meta element forces the page to be reloaded by Nav_push() and this does not call a_Nav_expect_done(). Unfortunately a_Nav_expect_done() is responsible for calling a_UIcmd_set_scroll_by_fragment(), so any page pulled in by Nav_push() does not scroll to the anchor.
Oops, I meant Nav_repush() instead of Nav_push(). Jeremy Henty
On Sun, Mar 23, 2008 at 11:37:35PM +0000, Jeremy Henty (me) wrote:
Any ideas how to fix this? I having a hard time getting my head around this bit of the code.
Hmm, this little patch seems to do it. Comments? Jeremy Henty
On Sun, Mar 23, 2008 at 11:57:12PM +0000, Jeremy Henty (me) wrote:
On Sun, Mar 23, 2008 at 11:37:35PM +0000, Jeremy Henty (me) wrote:
Any ideas how to fix this? I having a hard time getting my head around this bit of the code.
Hmm, this little patch seems to do it.
Ooops, no it doesn't - it crashes Dillo if you press the reload button or left-click an internal link on a page with a '<meta http-equiv="Content-Type"...' element. Strangely, middle-clicking to bring up a new browser is OK, which is why I missed this problem at first. So how *do* we fix this? Jeremy Henty
diff -pru -- 00_pre/src/nav.c 01_post/src/nav.c --- 00_pre/src/nav.c 2008-03-16 15:29:31.000000000 +0000 +++ 01_post/src/nav.c 2008-03-23 23:54:13.000000000 +0000 @@ -334,8 +334,9 @@ static void Nav_repush(BrowserWindow *bw ReqURL = a_Url_dup(a_History_get_url(NAV_TOP_UIDX(bw))); /* Let's make reload be from Cache */ a_Url_set_flags(ReqURL, URL_FLAGS(ReqURL) | URL_ReloadFromCache); + bw->nav_expect_url = ReqURL; + bw->nav_expecting = TRUE; Nav_open_url(bw, ReqURL, 0); - a_Url_free(ReqURL); } }
OK, I've figured out the problem and (as usual) my first thoughts were completely wrong. I'm not sure what the fix is, though. If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working. Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do. If it's not then I don't know what is: I already tried tweaking things so that the reload goes down the same code path as the initial load, but that just caused crashes everywhere. I'd really like to fix this as it regularly breaks pages for me, but I don't know the code well enough to sure I've got it right. Jeremy Henty
Hi Jeremy, On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
OK, I've figured out the problem and (as usual) my first thoughts were completely wrong. I'm not sure what the fix is, though.
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do. If it's not then I don't know what is: I already tried tweaking things so that the reload goes down the same code path as the initial load, but that just caused crashes everywhere.
I'd really like to fix this as it regularly breaks pages for me, but I don't know the code well enough to sure I've got it right.
Thanks for all the information. In fact, repush is an after thought, and some parts of nav also need some re-work. The API, IIRC pushing #fragment URLs to the stack has some anomalies too. It'd be good to find/design a single path to handle it (experience shows multiple paths is asking for trouble :). I've looked at it and it seems like deep analysis is required. I'm reviewing a few patches now. I'll work on it again when I find some time. -- Cheers Jorge.-
On Fri, Mar 28, 2008 at 09:57:10AM -0400, Jorge Arellano Cid wrote:
On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do.
In fact, repush is an after thought, and some parts of nav also need some re-work. The API, IIRC pushing #fragment URLs to the stack has some anomalies too. It'd be good to find/design a single path to handle it (experience shows multiple paths is asking for trouble :). I've looked at it and it seems like deep analysis is required.
Thanks for explaining this. I'll keep running with Layout::setAnchor(NULL) commented out for the moment and see how it goes. (Still no breakage yet.) I hope to be able to spend time looking at this further. If I have any thoughts about a good way to do all this I will post them. Unfortunately I have had to devote more time to earning a living recently (sigh) so I've spent less time looking at Dillo code than I would have liked. Regards, Jeremy Henty
Hi, On Fri, Mar 28, 2008 at 05:08:01PM +0000, Jeremy Henty wrote:
On Fri, Mar 28, 2008 at 09:57:10AM -0400, Jorge Arellano Cid wrote:
On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do.
In fact, repush is an after thought, and some parts of nav also need some re-work. The API, IIRC pushing #fragment URLs to the stack has some anomalies too. It'd be good to find/design a single path to handle it (experience shows multiple paths is asking for trouble :). I've looked at it and it seems like deep analysis is required.
Thanks for explaining this. I'll keep running with Layout::setAnchor(NULL) commented out for the moment and see how it goes. (Still no breakage yet.) I hope to be able to spend time looking at this further. If I have any thoughts about a good way to do all this I will post them. Unfortunately I have had to devote more time to earning a living recently (sigh) so I've spent less time looking at Dillo code than I would have liked.
After a long review of the code, I committed a patch that not only fixes things in nav.c but in other places which were also involved in the SEGFAULTs we were getting. Please test it Jeremy. Johannes, there's a patch for dw2 too. It involves minor initialization of scrollX and scrollY, and a printf. If I open dillo with /tmp, a wild value is got for scrollY: jcid@d620:~/C/Dillo/d2/dillo2-cur/src$ ./dillo-fltk /tmp/ dillo_dns_init: Here we go! (threaded) Enabling cookies as from cookiesrc... Nav_open_url: new url='file:/tmp/' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/' '>} adjustScrollPos: scrollX=0 scrollY=-155 ^^^^--- here! Note: my /tmp is shorter than the viewport. Please give it a look. -- Cheers Jorge.-
On Thu, Apr 03, 2008 at 06:16:03PM -0400, Jorge Arellano Cid wrote:
On Fri, Mar 28, 2008 at 05:08:01PM +0000, Jeremy Henty wrote:
On Fri, Mar 28, 2008 at 09:57:10AM -0400, Jorge Arellano Cid wrote:
On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
After a long review of the code, I committed a patch that not only fixes things in nav.c but in other places which were also involved in the SEGFAULTs we were getting. Please test it Jeremy.
Done. This fixes the anchor-breaking bug that I reported above. No regressions that I can see. Yay! Regards, Jeremy Henty -- If you aren't sure whether you can do this, just try it. The worst thing that can happen is that your computer could explode. -- The GIMP User Manual
Hi Jorge, On Thu, Apr 03, 2008 at 06:16:03PM -0400, Jorge Arellano Cid wrote:
Hi,
On Fri, Mar 28, 2008 at 05:08:01PM +0000, Jeremy Henty wrote:
On Fri, Mar 28, 2008 at 09:57:10AM -0400, Jorge Arellano Cid wrote:
On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do.
In fact, repush is an after thought, and some parts of nav also need some re-work. The API, IIRC pushing #fragment URLs to the stack has some anomalies too. It'd be good to find/design a single path to handle it (experience shows multiple paths is asking for trouble :). I've looked at it and it seems like deep analysis is required.
Thanks for explaining this. I'll keep running with Layout::setAnchor(NULL) commented out for the moment and see how it goes. (Still no breakage yet.) I hope to be able to spend time looking at this further. If I have any thoughts about a good way to do all this I will post them. Unfortunately I have had to devote more time to earning a living recently (sigh) so I've spent less time looking at Dillo code than I would have liked.
After a long review of the code, I committed a patch that not only fixes things in nav.c but in other places which were also involved in the SEGFAULTs we were getting. Please test it Jeremy.
Johannes, there's a patch for dw2 too. It involves minor initialization of scrollX and scrollY, and a printf. If I open dillo with /tmp, a wild value is got for scrollY:
The added initialization is good I think.
jcid@d620:~/C/Dillo/d2/dillo2-cur/src$ ./dillo-fltk /tmp/ dillo_dns_init: Here we go! (threaded) Enabling cookies as from cookiesrc... Nav_open_url: new url='file:/tmp/' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/' '>} adjustScrollPos: scrollX=0 scrollY=-155 ^^^^--- here!
Note: my /tmp is shorter than the viewport.
Please give it a look.
Funny, I had a similar effect in fltkviewport.cc some time ago. The problem becomes more obvious, when adding another printf at the beginning of adjustScrollPos (). Then we get: Nav_open_url: new url='file:/tmp/hu' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/hu' '>} adjustScrollPos: scrollX=0 scrollY=0 adjustScrollPos: scrollX=0 scrollY=-376 So its adjustScrollPos() itself that makes scrollY negative! The fix is to do the checks the other way round and not to use "else if" (see attached patch). I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position. Cheers, Johannes PS: I hope the patch format is ok like this. I'm using mercurial and the builtin diff may behave a bit differently than standard diff. Do you prefer inline or attached patches?
On Sat, Apr 05, 2008 at 11:58:45AM +0200, Johannes Hofmann wrote:
[...] jcid@d620:~/C/Dillo/d2/dillo2-cur/src$ ./dillo-fltk /tmp/ dillo_dns_init: Here we go! (threaded) Enabling cookies as from cookiesrc... Nav_open_url: new url='file:/tmp/' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/' '>} adjustScrollPos: scrollX=0 scrollY=-155 ^^^^--- here!
Note: my /tmp is shorter than the viewport.
Please give it a look.
Funny, I had a similar effect in fltkviewport.cc some time ago. The problem becomes more obvious, when adding another printf at the beginning of adjustScrollPos (). Then we get:
Nav_open_url: new url='file:/tmp/hu' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/hu' '>} adjustScrollPos: scrollX=0 scrollY=0 adjustScrollPos: scrollX=0 scrollY=-376
So its adjustScrollPos() itself that makes scrollY negative! The fix is to do the checks the other way round and not to use "else if" (see attached patch).
Good it wasn't a complex side effect. Committed.
I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position.
I'll check it.
PS: I hope the patch format is ok like this. I'm using mercurial and the builtin diff may behave a bit differently than standard diff.
I like "diff -pru", because of the context, and the explicit stating of the function where the changes happen. Mercurial seems to provide both (as in your attached patch).
Do you prefer inline or attached patches?
Attached. -- Cheers Jorge.-
Hi, On Sat, Apr 05, 2008 at 11:58:45AM +0200, Johannes Hofmann wrote:
[...] I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position.
I checked with a dillo version even as old as 2007-11-23 and the bug is present. So it's not related to the newest anchor patch. It seems somewhere the offset is not being updated. If I load the page from cache, it works! i.e. file:/tmp/44240.html file:/tmp/44240.html#entry-44240 works OK. Using a cgi to simulate network transfer rate (10KB/sec), I can reproduce the bug: http://localhost/cgi-bin/slow.cgi?10000:/tmp/44240.html#entry-44240 That's what I've found so far. -- Cheers Jorge.-
Hi,
[Johannes wrote:] I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position.
There were mixed issues here. I just committed a patch that modifies/fixes anchor handling with repush (on the last weeks anchors didn't work for me even inside the bookmarks page). Now it works, and more like Firefox. This is, now reloading a page with fragment, scrolls to the fragment position, but going back and forward remembers the last user-scrolled position; quite comfortable. The page you mention, works OK when it's a local file, but not from the network. This looks like a bug hiding in the offset calculation or scroll adjustment while the page loads images. In brief: with the patch the situation improves a lot, and there's still a bug to fix. -- Cheers Jorge.-
Jorge wrote:
[Johannes wrote:] I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position.
There were mixed issues here.
I just committed a patch that modifies/fixes anchor handling with repush (on the last weeks anchors didn't work for me even inside the bookmarks page).
Now it works, and more like Firefox. This is, now reloading a page with fragment, scrolls to the fragment position, but going back and forward remembers the last user-scrolled position; quite comfortable.
The page you mention, works OK when it's a local file, but not from the network. This looks like a bug hiding in the offset calculation or scroll adjustment while the page loads images.
In brief: with the patch the situation improves a lot, and there's still a bug to fix.
Things seem flaky now with redirection. Here's what happens if I 1 type in dillo.org 2 type in minix3.org $ dillo dillo_dns_init: Here we go! (threaded) Disabling cookies. Nav_open_url: new url='about:blank' Nav_open_url: new url='http://dillo.org' Dns_server [0]: dillo.org is 0x8171648 Connecting to 134.102.206.165 Nav_open_url: new url='http://www.dillo.org/' Dns_server [0]: www.dillo.org is 0x8194228 Connecting to 134.102.206.165 META Content-Type changes charset to: iso-8859-1 Nav_open_url: new url='http://www.dillo.org/' META Content-Type gave charset as: iso-8859-1 a_Nav_expect_done: repush! Nav_open_url: new url='http://minix3.org' Dns_server [0]: minix3.org is 0x8147220 Connecting to 130.37.20.20 Nav_open_url: new url='http://www.minix3.org/' Dns_server [0]: www.minix3.org is 0x8198090 Connecting to 130.37.20.20 a_Nav_expect_done: e2ereload! META Content-Type changes charset to: iso-8859-1 Nav_open_url: new url='http://www.dillo.org/' META Content-Type gave charset as: iso-8859-1 a_Nav_expect_done: repush! I did not press Back.
Hi, On Thu, May 08, 2008 at 09:25:59PM +0000, corvid wrote:
Jorge wrote:
[Johannes wrote:] I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position.
There were mixed issues here.
I just committed a patch that modifies/fixes anchor handling with repush (on the last weeks anchors didn't work for me even inside the bookmarks page).
Now it works, and more like Firefox. This is, now reloading a page with fragment, scrolls to the fragment position, but going back and forward remembers the last user-scrolled position; quite comfortable.
The page you mention, works OK when it's a local file, but not from the network. This looks like a bug hiding in the offset calculation or scroll adjustment while the page loads images.
In brief: with the patch the situation improves a lot, and there's still a bug to fix.
Things seem flaky now with redirection.
Here's what happens if I 1 type in dillo.org 2 type in minix3.org
$ dillo dillo_dns_init: Here we go! (threaded) Disabling cookies. Nav_open_url: new url='about:blank' Nav_open_url: new url='http://dillo.org' Dns_server [0]: dillo.org is 0x8171648 Connecting to 134.102.206.165 Nav_open_url: new url='http://www.dillo.org/' Dns_server [0]: www.dillo.org is 0x8194228 Connecting to 134.102.206.165 META Content-Type changes charset to: iso-8859-1 Nav_open_url: new url='http://www.dillo.org/' META Content-Type gave charset as: iso-8859-1 a_Nav_expect_done: repush! Nav_open_url: new url='http://minix3.org' Dns_server [0]: minix3.org is 0x8147220 Connecting to 130.37.20.20 Nav_open_url: new url='http://www.minix3.org/' Dns_server [0]: www.minix3.org is 0x8198090 Connecting to 130.37.20.20 a_Nav_expect_done: e2ereload! META Content-Type changes charset to: iso-8859-1 Nav_open_url: new url='http://www.dillo.org/' META Content-Type gave charset as: iso-8859-1 a_Nav_expect_done: repush!
I did not press Back.
Oh yes. I didn't want to dig deep into it, but after this there was no chance to hide. :) There was a semantic confussion with the E2EReload flag, which was split into URL_E2EQuery and URL_ReloadPage. Now we can tell the difference between an first-time E2EQuery and an E2E reload. Patch committed. There's still a bug hiding, most probably inside the code setting the scroll position in dw2. When you have a page with repush (i.e. with charset specified in META element, e.g. www.dillo.org :), and you scroll the page, and then reload. It will be shown from the origin, even though a_UIcmd_set_scroll_xy is called from nav.c:321 with the right coords (enable the _MSG to check). -- Cheers Jorge.-
Jorge wrote:
There's still a bug hiding, most probably inside the code setting the scroll position in dw2. When you have a page with repush (i.e. with charset specified in META element, e.g. www.dillo.org :), and you scroll the page, and then reload. It will be shown from the origin, even though a_UIcmd_set_scroll_xy is called from nav.c:321 with the right coords (enable the _MSG to check).
Scrolled down a little and hit reload. In Layout::adjustScrollPos(), scrollY of 60 became 0 because canvasAscent == 5 and canvasDescent == 5. Looking at the backtrace, a_UIcmd_set_scroll_xy() from a_Nav_expect_done() from a_Web_dispatch_by_type() would seem to be too early.
On Sat, May 10, 2008 at 09:11:27PM +0000, corvid wrote:
Jorge wrote:
There's still a bug hiding, most probably inside the code setting the scroll position in dw2. When you have a page with repush (i.e. with charset specified in META element, e.g. www.dillo.org :), and you scroll the page, and then reload. It will be shown from the origin, even though a_UIcmd_set_scroll_xy is called from nav.c:321 with the right coords (enable the _MSG to check).
Scrolled down a little and hit reload. In Layout::adjustScrollPos(), scrollY of 60 became 0 because canvasAscent == 5 and canvasDescent == 5. Looking at the backtrace, a_UIcmd_set_scroll_xy() from a_Nav_expect_done() from a_Web_dispatch_by_type() would seem to be too early.
AFAIR it was handled with an idle function in dw1 in dillo1. Now it's also handled with an idle function, and I seem to recall Sebastian told me something about anchors long ago, but I don't remember. It is tricky because you have to handle several cases, with some events. For instance: * When the page is still loading and you have an anchor to scroll to. Here you have to retry until the page loads enough to scroll to the anchor, and you should keep watching just in case an image placed before the anchor arrives later, and then adjust again. Now if the user decides to scroll with the mouse, before the position settles, then you have to abort the idle scrolling, and let him control it. All this seasoned with window resize, page changes and other events. :-) -- Cheers Jorge.-
participants (4)
-
corvid＠lavabit.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org