Skip to content

PgUP/PgDn and space key #68

Closed
aakos23 opened this Issue Apr 4, 2012 · 30 comments

6 participants

@aakos23
aakos23 commented Apr 4, 2012

I push these button:

Error: in bind call: /home/user/.config/luakit/webview.lua:341: attempt to perform arithmetic on local 'p' (a nil value)

Used: luakit develop package

@Donearm
Donearm commented Apr 8, 2012

Also Ctrl+u and Ctrl+d show the same error message. Basically only hjkl works. Using luakit 2012.03.25

@ymln
ymln commented Apr 22, 2012

I also have this issue on Arch Linux in luakit 2012.03.25 with locale ru_UA.UTF-8
git bisect shows that bug was introduced in commit 403aaff
It seems that Lua's tonumber function is locale-specific. In my locale comma is used as decimal separator instead of dot, so tonumber("+1.0") returns nil.
Starting Luakit like this:

LANG=C luakit

seems to "fix" the problem.
By the way, I can reproduce this bug only after visiting some pages, like youtube video pages or github. After restarting luakit everything works fine again.

@Plaque-fcc

SOLUTION DEPRECATED

Can confirm for ru_RU.UTF-8; using luakit from git master branch.
This is a rough (but better than none) workaround fallback down here. Keep in mind that I don't know Lua at all, just wanna get rid of the bug; please improve the code if you can.

diff for webview.lua:
353c353,362

<                 if n then scroll[axis] = func(scroll, axis, tonumber(n)) end
---
>                 if n then
>                     local num = tonumber(n)
>                     if not num then
>                         -- We obviously got some non-dot float point locale
>                         local num_str, match_times = string.gsub(n, "([-+]%d+)[,.]?(%d+)", "%1,%2")
>                         num = tonumber( num_str )
>                         -- Rough but workable.
>                     end
>                     scroll[axis] = func(scroll, axis, num)
>                 end

Reproduced the bug using the very this issue page (sic!). Before I load it, tonumber() works normally, and since tonumber() changes its behaviour, it's on, every time, on every tab, every URI.
And yes I know the solution can slow luakit down on scrolling by page, but yet no error message seen since the changes made.

@Plaque-fcc

Hey, we don't have to care do local settings apply or not, even, correct?

Look, more proper patch (diff for webview.lua):

335c335
<     ["^(%d+%.?%d*)p$"] = function (s, axis, p)
---
>     ["^(%d+[.,]?%d*)p$"] = function (s, axis, p)
340c340
<     ["^([-+]%d+%.?%d*)p$"] = function (s, axis, p)
---
>     ["^([-+]%d+[.,]?%d*)p$"] = function (s, axis, p)

(diff for binds.lua):

123,129c123,129
<     key({"Control"}, "d",           function (w) w:scroll{ y = "+0.5p" } end),
<     key({"Control"}, "u",           function (w) w:scroll{ y = "-0.5p" } end),
<     key({"Control"}, "f",           function (w) w:scroll{ y = "+1.0p" } end),
<     key({"Control"}, "b",           function (w) w:scroll{ y = "-1.0p" } end),
<     key({},          "space",       function (w) w:scroll{ y = "+1.0p" } end),
<     key({"Shift"},   "space",       function (w) w:scroll{ y = "-1.0p" } end),
<     key({},          "BackSpace",   function (w) w:scroll{ y = "-1.0p" } end),
---
>     key({"Control"}, "d",           function (w) w:scroll{ y = string.format("+%.1fp", 0.5) } end),
>     key({"Control"}, "u",           function (w) w:scroll{ y = string.format("-%.1fp", 0.5) } end),
>     key({"Control"}, "f",           function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
>     key({"Control"}, "b",           function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
>     key({},          "space",       function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
>     key({"Shift"},   "space",       function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
>     key({},          "BackSpace",   function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
140,141c140,141
<     key({},          "Page_Down",   function (w) w:scroll{ y = "+1.0p" } end),
<     key({},          "Page_Up",     function (w) w:scroll{ y = "-1.0p" } end),
---
>     key({},          "Page_Down",   function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
>     key({},          "Page_Up",     function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),

The idea is to let the interpreters depend on locales or not, but both produce and parse floatpoint values within the same conversion rules. [^.^]

@Plaque-fcc

ymln, noticed that changing to locale-dependent tonumber() behaviour occurs approx. after

** (luakit:1978): DEBUG: NP_Initialize
** (luakit:1978): DEBUG: NP_Initialize succeeded

is written to stdout four times consequently.
Got no idea why but something's whispering me it's possibly a related thing.

@xcfw
xcfw commented May 18, 2012

My webview.lua is like this:

for 345:357 line

function webview.methods.scroll(view, w, new)
    local scroll = view.scroll
    for axis, val in pairs{ x = new.x, y = new.y } do
        if type(val) == "number" then
            scroll[axis] = val
        else
            for pat, func in pairs(webview.scroll_parse_funcs) do
                local n = string.match(val, pat)
                if n then scroll[axis] = func(scroll, axis, tonumber(n)) end
            end
        end
    end
end

I've changed webview.lua and binds.lua as it shown on comment of Plaque-fcc above, but thtere is no result.

My version of luakit is: luakit 2012.03.25-8-gf8158a1
ArchLinux 3.3.6-1-ARCH #1 SMP PREEMPT i686 GNU/Linux

@Plaque-fcc

In your ~/.config/luakit/ in file binds.lua must be placed

     key({},          "Page_Down",   function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
     key({},          "Page_Up",     function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),

instead of

     key({},          "Page_Down",   function (w) w:scroll{ y = "+1.0p" } end),
     key({},          "Page_Up",     function (w) w:scroll{ y = "-1.0p" } end),

And in your webview.lua:

     ["^([-+]%d+[.,]?%d*)p$"] = function (s, axis, p)

instead of

     ["^([-+]%d+%.?%d*)p$"] = function (s, axis, p)

Please, make sure it's so. (It's solved the issue's source for me.)

```$ dpkg-query --showformat='${PackageSpec}\t${Version}\t${Status}\n' --show libwebkit* libglib2.0-0 liblua5.1* libpango1.0-0 libsoup2.4* libunique-1.0* | grep ' installed'
libglib2.0-0 2.30.0-0ubuntu4 install ok installed
liblua5.1-0 5.1.4-10 install ok installed
liblua5.1-0-dev 5.1.4-10 install ok installed
liblua5.1-filesystem0 1.5.0-2 install ok installed
liblua5.1-socket-dev 2.0.2-6 install ok installed
liblua5.1-socket2 2.0.2-6 install ok installed
liblua5.1-sql-sqlite3-2 2.2.0~rc1-4 install ok installed
liblua5.1-sql-sqlite3-dev 2.2.0~rc1-4 install ok installed
libpango1.0-0 1.29.3+git20110916-0ubuntu1 install ok installed
libsoup2.4-1 2.36.0-0ubuntu1 install ok installed
libsoup2.4-dev 2.36.0-0ubuntu1 install ok installed
libunique-1.0-0 1.1.6-2ubuntu1 install ok installed
libwebkit-dev 1.4.3-0ubuntu4 install ok installed
libwebkitgtk-1.0-0 1.4.3-0ubuntu4 install ok installed
libwebkitgtk-1.0-common 1.4.3-0ubuntu4 install ok installed
libwebkitgtk-3.0-0 1.4.3-0ubuntu4 install ok installed
libwebkitgtk-3.0-common 1.4.3-0ubuntu4 install ok installed
libwebkitgtk-dev 1.4.3-0ubuntu4 install ok installed

@Plaque-fcc

mason-larobina, I'm gonna make a pull request on it.

@Plaque-fcc Plaque-fcc pushed a commit that referenced this issue Jun 14, 2012
Plaque FCC To fix issue mason-larobina/luakit#68 with locale-dependent Lua 'tonu…
…mber()' behaviour.

Signed-off-by: Plaque FCC <plaque-fcc@github>
5cc2ef8
@Plaque-fcc Plaque-fcc pushed a commit that referenced this issue Jun 14, 2012
Plaque FCC To fix issue mason-larobina/luakit#68 with locale-dependent Lua 'tonu…
…mber()' behaviour.

Finally the proper commit for a pull request.

Signed-off-by: Plaque FCC <Plaque-fcc@github>
ac997c6
@Plaque-fcc

Now should be okay and clean. Affects 'develop' branch.

@mason-larobina
Owner

Now I'm wondering if #91 will help while browsing sites of different locales. We could end up in the reversed situation of the "+1,0p" format not working when visiting en_US and other "1.0" locales. Would anyone mind giving this patch a bit of a field test as I'm not sure of the exact circumstances which cause libwebkit to change the locale at the application level (seems stupid to me).

@Plaque-fcc

Hey, maybe it's clear to track, but I still do instist that it's quite a rude solution, because one needs to change some string constant each time into different code, not an appropriate single function call, tell me I'm wrong here?
And I'd say this should not be included into the code if there was a better piece of code doing the same thing, but I don't see one.

Why I'd have this included at the moment? As it was said earlier in this issue's comments, there is a [bug?] in Lua behaviour on my system which leads luakit to parse "+1.0p" string by a pattern quite sane for a while and then SUDDENLY® start to show this fail message because of Lua 'tonumber()' itoa/atoi rules change.
Those who see the error use to report that they use non-en_XX locale just like I do [mine is ru_RU.UTF-8], and SUDDENLY© Lua cease to understand dot currency fraction separator in textual representations of numbers switching to comma way. Here is why the patch is.

So far, it's not about website's locale, I suppose. It's about environment's locale.

@Plaque-fcc

Need a fast proof?

  • switch to a locale like ru_RU.UTF-8;
  • load something like google search page long enough to scroll page up/down obviously;
  • ensure that without this patch everything like pgUp/pgDn works as it's supposed to;
  • load this issue page completely;
  • now check pgUp/pgDn again; you may be surprised, you know.

Then test everything again with the patch and tell us how does it feel.

@Plaque-fcc

How does the patch do the trick?
The way 'tonumber()' treats numeric string constants runs the same way 'string.format()' does, so each time any ("+1" .. separator .. "0p") is being formed, 'tonumber()' will understand the same separator, you see? There is no need to have this patch for that segment of users don't encounter such rules switching because they don't use locales which have other than dot fraction separator, but there is no regression in having this patch for everyone, just because there are machines being used by several users setting their environment to different languages; thus some of them may encounter the problem that we did not prepare for such particular case yet confirmed to appear. It's like there are thousands of people living by UTC+0, but this does not imply there's no need to provide UTC+N shifting capabilities for other people using the same software, yeah?

More on the bug: once revealed, it stays with you all the way before you restart luakit. What do you think?

@Plaque-fcc

And this may be caused by loading some javascript, did not you think about it?

If the people affected by the issue could provide the URIs after visiting them leading luakit to fail, we could dig further and eliminate the very problem's source itself. Because, you know, I got surprised sadly with this interpretation rules change on the fly and not reverting back some time later, as it could be gone when some webpage content is gone.

@ymln
ymln commented Jun 15, 2012

Weird, I think I finally found when locale is changed to default by webkit. It happens when this JS code is executed by the browser:

document.createElement('audio');

So to test it just create HTML file with contents:

<script type="text/javascript">
    document.createElement('audio');
</script>

and open it in luakit.

Any ideas why this might happen? Maybe it's a bug in webkit?

@Plaque-fcc

I dare think, you're right. You took the lead about maybe-its-about-javascript and analysed this site, did not you?
Moreover, there was an issue about broken «follow» mechanism when upgrade to webkit 1.8.0 which I have to confirm.

Don't even know which way should we turn now: either produce bugcovering workaround which can break performance and lead to severe compromise or should we involve the webkit team?

About possible "audio": tested with and without pulseaudio audio router: all the same.

@ymln
ymln commented Jun 16, 2012
@Plaque-fcc
@ymln
ymln commented Jun 16, 2012
@Plaque-fcc

You can see when you hover your mouse pointer there above «Close Issue» button and below tour message timestamp two buttons: «Edit» and «Delete» right against two first rows of each your message, and they hide as you hover off.

On 403aaff: first glance says yes, you're right, it could avoid locale switch affection. But saying explicitly we want to scroll plus 1.0 page I like, while reverting from "+1.0p" format back means API degradation. Should (1 + 0.0) and (1) be equal? They should be, by value, but not by their string representation.
And it all reminds me MS Office 2007 not-so-obvious bug with the same locale number format, yeah, which can't be legally investigated (it does not show what's wrong, just complains about something_wrong_somewhere) and fixed by an ordinary user.

The method using os.setlocale("C") is just another suppression like LANG=C luakit mentioned here above. Don't think that turning rules off is the only thing we would desire here.

@Plaque-fcc

Just look at what came along this commit and behold:

  • w:scroll_vert() -w:scroll_horiz() -w:scroll_page()

At least three similar functions were replaced with single w:scroll(), more flexible and universal, so you can w:scroll({ x = something, y = something }), can't you?

@mason-larobina
Owner

Maybe a simple solution is in order. How about a class of fields like w:scroll{ xrel = 4.0, y = 10, ypage = 2.0, xpagerel = 2.0 }

@ymln
ymln commented Jun 17, 2012
@mason-larobina
Owner

Could sombody that can replicate the problem try this patch out (apply on to current develop head). This uses the x, xrel, xpage, xpagerel, xpct, y... table fields for scrolling and should be quite a bit faster than the regex method.
Plus I don't think it looks that bad.

diff --git a/config/binds.lua b/config/binds.lua
index 8ff47e5..9955eca 100644
--- a/config/binds.lua
+++ b/config/binds.lua
@@ -12,7 +12,6 @@ local strip, split = lousy.util.string.strip, lousy.util.string.split

 -- Globals or defaults that are used in binds
 local scroll_step = globals.scroll_step or 20
-local more, less = "+"..scroll_step.."px", "-"..scroll_step.."px"
 local zoom_step = globals.zoom_step or 0.1

 -- Add binds to a mode
@@ -112,39 +111,44 @@ add_binds("normal", {
     key({},          ":",           function (w) w:set_mode("command") end),

     -- Scrolling
-    key({},          "j",           function (w) w:scroll{ y = more    } end),
-    key({},          "k",           function (w) w:scroll{ y = less    } end),
-    key({},          "h",           function (w) w:scroll{ x = less    } end),
-    key({},          "l",           function (w) w:scroll{ x = more    } end),
-    key({},          "^",           function (w) w:scroll{ x = "0%"    } end),
-    key({},          "$",           function (w) w:scroll{ x = "100%"  } end),
-    key({"Control"}, "e",           function (w) w:scroll{ y = more    } end),
-    key({"Control"}, "y",           function (w) w:scroll{ y = less    } end),
-    key({"Control"}, "d",           function (w) w:scroll{ y = string.format("+%.1fp", 0.5) } end),
-    key({"Control"}, "u",           function (w) w:scroll{ y = string.format("-%.1fp", 0.5) } end),
-    key({"Control"}, "f",           function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
-    key({"Control"}, "b",           function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
-    key({},          "space",       function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
-    key({"Shift"},   "space",       function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
-    key({},          "BackSpace",   function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
+    key({},          "j",           function (w) w:scroll{ yrel =  scroll_step } end),
+    key({},          "k",           function (w) w:scroll{ yrel = -scroll_step } end),
+    key({},          "h",           function (w) w:scroll{ xrel = -scroll_step } end),
+    key({},          "l",           function (w) w:scroll{ xrel =  scroll_step } end),
+    key({},          "Down",        function (w) w:scroll{ yrel =  scroll_step } end),
+    key({},          "Up",          function (w) w:scroll{ yrel = -scroll_step } end),
+    key({},          "Left",        function (w) w:scroll{ xrel = -scroll_step } end),
+    key({},          "Right",       function (w) w:scroll{ xrel =  scroll_step } end),
+
+    key({},          "^",           function (w) w:scroll{ x =  0 } end),
+    key({},          "$",           function (w) w:scroll{ x = -1 } end),
+    key({},          "0",           function (w, m)
+                                        if not m.count then w:scroll{ y = 0 } else return false end
+                                    end),

+
+    key({"Control"}, "e",           function (w) w:scroll{ yrel =  scroll_step } end),
+    key({"Control"}, "y",           function (w) w:scroll{ yrel = -scroll_step } end),
+    key({"Control"}, "d",           function (w) w:scroll{ ypagerel =  0.5 } end),
+    key({"Control"}, "u",           function (w) w:scroll{ ypagerel = -0.5 } end),
+    key({"Control"}, "f",           function (w) w:scroll{ ypagerel =  1.0 } end),
+    key({"Control"}, "b",           function (w) w:scroll{ ypagerel = -1.0 } end),
+
+    key({},          "space",       function (w) w:scroll{ ypagerel =  1.0 } end),
+    key({"Shift"},   "space",       function (w) w:scroll{ ypagerel = -1.0 } end),
+    key({},          "BackSpace",   function (w) w:scroll{ ypagerel = -1.0 } end),
+
+    key({},          "Page_Down",   function (w) w:scroll{ ypagerel =  1.0 } end),
+    key({},          "Page_Up",     function (w) w:scroll{ ypagerel = -1.0 } end),
+
+    key({},          "Home",        function (w) w:scroll{ y =  0 } end),
+    key({},          "End",         function (w) w:scroll{ y = -1 } end),

     -- Specific scroll
-    buf("^gg$",                     function (w, b, m) w:scroll{ y = m.count.."%" } end, {count = 0}),
-    buf("^G$",                      function (w, b, m) w:scroll{ y = m.count.."%" } end, {count = 100}),
+    buf("^gg$",                     function (w, b, m) w:scroll{ ypct = m.count } end, {count = 0}),
+    buf("^G$",                      function (w, b, m) w:scroll{ ypct = m.count } end, {count = 100}),
+    buf("^%%$",                     function (w, b, m) w:scroll{ ypct = m.count } end),

     -- Traditional scrolling commands
-    key({},          "Down",        function (w) w:scroll{ y = more    } end),
-    key({},          "Up",          function (w) w:scroll{ y = less    } end),
-    key({},          "Left",        function (w) w:scroll{ x = less    } end),
-    key({},          "Right",       function (w) w:scroll{ x = more    } end),
-    key({},          "Page_Down",   function (w) w:scroll{ y = string.format("+%.1fp", 1.0) } end),
-    key({},          "Page_Up",     function (w) w:scroll{ y = string.format("-%.1fp", 1.0) } end),
-    key({},          "Home",        function (w) w:scroll{ y = "0%"    } end),
-    key({},          "End",         function (w) w:scroll{ y = "100%"  } end),
-    key({},          "$",           function (w) w:scroll{ x = "100%"  } end),
-    key({},          "0",           function (w, m)

-                                        if not m.count then w:scroll{ y = "0%" } else return false end
-                                    end),

     -- Zooming
     key({},          "+",           function (w, m)    w:zoom_in(zoom_step  * m.count)       end, {count=1}),
diff --git a/config/webview.lua b/config/webview.lua
index 4d5ad70..22534f3 100644
--- a/config/webview.lua
+++ b/config/webview.lua
@@ -290,44 +290,33 @@ webview.methods = {
     end,
 }

-webview.scroll_parse_funcs = {
-    -- Abs "100px"
-    ["^(%d+)px$"] = function (_, _, px) return px end,
-
-    -- Rel "+/-100px"
-    ["^([-+]%d+)px$"] = function (s, axis, px) return s[axis] + px end,
-
-    -- Abs "10%"
-    ["^(%d+)%%$"] = function (s, axis, pc)
-        return math.ceil(s[axis.."max"] * (pc / 100))
-    end,
-
-    -- Rel "+/-10%"
-    ["^([-+]%d+)%%$"] = function (s, axis, pc)
-        return s[axis] + math.ceil(s[axis.."max"] * (pc / 100))
-    end,
-
-    -- Abs "10p" (pages)
-    ["^(%d+[%.,]?%d*)p$"] = function (s, axis, p)
-        return math.ceil(s[axis.."page_size"] * p)
-    end,
-
-    -- Rel "+10p" (pages)
-    ["^([-+]%d+[%.,]?%d*)p$"] = function (s, axis, p)
-        return s[axis] + math.ceil(s[axis.."page_size"] * p)
-    end,
-}
-
 function webview.methods.scroll(view, w, new)
-    local scroll = view.scroll
-    for axis, val in pairs{ x = new.x, y = new.y } do
-        if type(val) == "number" then
-            scroll[axis] = val
-        else
-            for pat, func in pairs(webview.scroll_parse_funcs) do
-                local n = string.match(val, pat)
-                if n then scroll[axis] = func(scroll, axis, tonumber(n)) end
+    local s = view.scroll
+    for _, axis in ipairs{ "x", "y" } do
+        -- Relative px movement
+        if rawget(new, axis.."rel") then
+            s[axis] = s[axis] + new[axis.."rel"]
+
+        -- Relative page movement
+        elseif rawget(new, axis .. "pagerel") then
+            s[axis] = s[axis] + math.ceil(s[axis.."page_size"] * new[axis.."pagerel"])
+
+        -- Absolute px movement
+        elseif rawget(new, axis) then
+            local n = new[axis]
+            if n == -1 then
+                s[axis] = s[axis.."max"]
+            else
+                s[axis] = n
             end
+
+        -- Absolute page movement
+        elseif rawget(new, axis.."page") then
+            s[axis] = math.ceil(s[axis.."page_size"] * new[axis.."page"])
+
+        -- Absolute percent movement
+        elseif rawget(new, axis .. "pct") then
+            s[axis] = math.ceil(s[axis.."max"] * (new[axis.."pct"]/100))
         end
     end
@Plaque-fcc
@mason-larobina
Owner

That was an oversight on my part, didn't notice I'd added it twice. It's removed in the above patch and if it solves our problem I'm going to push it.

@Plaque-fcc

There's something wrong with me or your patch, because it's getting rejected on binds farther than one line.
Switching to manual merge mode; takes time.

Upd0: merged, testing.

This patch completely avoids using string conversion, so there'll be nothing about 'nil number' error.

@Plaque-fcc

Feels like many of the scrolls work finely as supposed to.
I support this commit. (%

@Plaque-fcc

Thus, as soon as you commit, this issue should be considered as closed.

@mason-larobina
Owner

Ok pushed (33aedf6)

Issue resolved. Thanks for the input & testing guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.