Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ButtonTable: taller buttons for easier tap #5554

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@poire-z
Copy link
Contributor

poire-z commented Oct 31, 2019

See #5449 (comment). I went with Screen:scaleBySize(4),, which is between the 2nd (2) and the 3rd (5) options among the screenshots there.
Went with 4 so I have no regret not aiming high - we'll decrease it to 3 (or 2 if really needed) if people find that too big.
Closes #5449.

@Frenzie Frenzie added this to the 2019.11 milestone Oct 31, 2019
@@ -50,6 +50,7 @@ local Size = {
small = Screen:scaleBySize(2),
large = Screen:scaleBySize(10),
button = Screen:scaleBySize(2),
buttontable = Screen:scaleBySize(4),

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 31, 2019

Member

Alternatively, increase button?

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Author Contributor

There are so many things that are Buttons (that don't look like Buttons :) but just need to be tap'able - like our chevrons, the Dict lookup result word...) that would then be changed and made taller, resulting in taller everything :) that would really be noticable.
I don't remember having any tap issue with other things than these ButtonTable - so I'd rather not change regular Buttons.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 31, 2019

Member

Well, besides the "real" buttons, doesn't that logically mean that the buttons that don't look like buttons are hard to tap too? :-)

I'm more immediately concerned about consistency with other buttons in general though.

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Author Contributor

Logically, less hard (cause they got 2) than ButtonTables (who got 0).
But by experience, no issue.
"Other" buttons may also have a bigger font size, or some increase paddings.
We'll see how we feel about other Buttons now being less tall than ButtonTables/ButtonDialogs - but for now, I don't want to engage in a global UI entall'enment :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 31, 2019

Member

@NiLuJe Any opinion?

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Oct 31, 2019

Member

I'm with @poire-z, the only potentially annoying to hit buttons are always on a button table, AFAIK.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 31, 2019

Member

The only buttons I ever misclick (but instead accidentally dismiss a dialog) are not on a ButtonTable. :-P

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Author Contributor

Just to be clear: buttons at the bottom of ConfirmBox are in a ButtonTable, and affected by my changes. They are all taller now (the only one bothering me a bit is the View HTML text viewer, with its View xxx.css, View yyy.css, Switch to debug view, Close, each alone on its own line - which takes a bit of vertical space out of the HTML text - but I'll be fine with it).

(The other "buttons" I can think of in a dialog are may be the +1 -1 << in the Frontlight widget or the SkimTo widget , and they are quite far from the borders to cause a dismiss if missed :)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Merging, so I can see how it looks on Android - and get feedback, and reduce it, and get done with it :)

@poire-z poire-z merged commit eb8795e into koreader:master Nov 1, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:buttonttable_more_padding branch Nov 1, 2019
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

My feedback: I'd rather just have the existing .button height.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

On which device have you checked?
(I found it really fine on my Android phone.)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

H2O, Android, desktop... the device doesn't matter. It's just unbalanced.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Unbalanced with/vs what?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

Mainly other padding, but also the size of the text. It's just too big and ugly. :-/

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

It's just too big and ugly. :-/

OK, this wording, I understand :)
(I found it ugly for about an hour yesterday; then I got used to it :)

We're still mostly on the side of good rather than bad, when comparing to your screenshots - and comparing ButtonTable to our Menu (that this PR didn't touch):

image

This was the look before this PR:
image

And this would be the look with buttontable = Screen:scaleBySize(2), (same value as button=)
image

Would you care checking how you'd find the look with buttontable = Screen:scaleBySize(3),?
I won't be bothered if we end up using buttontable = button = Screen:scaleBySize(2), - but I think using a bit more improves UX.

Regarding your suggestion from yesterday about increasing the generic button = Screen:scaleBySize(2),, and using 6 to exagerate it (only a bit), here are some of the the ugly things that stand out (may be because we didn't do all that layout well - but I don't want to be the one fixing them :)
image
image
image
image

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

3 is less bad of course, but 2 is perfect. 1 is better than 3, 0 is and always has been too small and unbalanced. But I think we very much shouldn't confuse the visual display with the touch target.

Regarding your suggestion from yesterday about increasing the generic button = Screen:scaleBySize(2),

Not a suggestion really; I think 2 is good and I don't think a separate buttontable size makes any sense. But if you adjust this padding, you need to adjust a lot more paddings and margins to maintain visual unity.

Right now a button is 32 or 34 dp high (depending on border). You want to make them 38. This is not by itself problematic, but doing it in isolation is unbalanced and ugly.

I believe what you really want is to have a larger tap area. Something like this (2-minute concept so ugly and accidentally killing something in the process but hopefully it gets the point across):

Screenshot_2019-11-01_15-52-14

diff --git a/frontend/ui/widget/buttontable.lua b/frontend/ui/widget/buttontable.lua
index 676a1661..08bc7370 100644
--- a/frontend/ui/widget/buttontable.lua
+++ b/frontend/ui/widget/buttontable.lua
@@ -57,13 +57,15 @@ function ButtonTable:init()
                 hold_callback = btn_entry.hold_callback,
                 width = (self.width - sizer_space)/column_cnt,
                 max_width = (self.width - sizer_space)/column_cnt - 2*self.sep_width - 2*self.padding,
-                bordersize = 0,
-                margin = 0,
-                padding = Size.padding.buttontable, -- a bit taller than standalone buttons, for easier tap
+                --~ bordersize = 0,
+                bordersize = 1,
+                margin = Size.padding.button*2,
+                padding = Size.padding.button, -- a bit taller than standalone buttons, for easier tap
                 padding_h = 0, -- allow text to take more of the horizontal space
                 text_font_face = self.button_font_face,
                 text_font_size = self.button_font_size,
                 show_parent = self.show_parent,
+                radius = 1,
             }
             if btn_entry.id then
                 self.button_by_id[btn_entry.id] = button
@@ -78,14 +80,14 @@ function ButtonTable:init()
             }
             buttons_layout_line[j] = button
             table.insert(horizontal_group, button)
-            if j < column_cnt then
-                table.insert(horizontal_group, vertical_sep)
-            end
+            --~ if j < column_cnt then
+                --~ table.insert(horizontal_group, vertical_sep)
+            --~ end
         end -- end for each button
         table.insert(self.container, horizontal_group)
-        if i < row_cnt then
-            self:addHorizontalSep(true, true, true)
-        end
+        --~ if i < row_cnt then
+            --~ self:addHorizontalSep(true, true, true)
+        --~ end
         if column_cnt > 0 then
             --Only add line that are not separator to the focusmanager
             table.insert(self.buttons_layout, buttons_layout_line)

Compare how the Android Material Design spec says a button should be 36 dp high in display and 48 in touch target.

https://material.io/design/layout/spacing-methods.html#touch-targets

Screenshot_2019-11-01_15-55-48

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

But with a 3x3 button table, without any border/spacing between buttons, how do you want to increase the tap area for the middle line buttons ? They would overlap.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

My screenshot is about the way the buttons are displayed (with margin separation for visual clarity and touch space extension), the 3x3 is just an accident.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

the 3x3 is just an accident

Yes, but when we have a button table like we do, with stuck buttons without any margin, the fact that you can't extend the tap area without any overlap/conflict (which has precedence? these shouldn't be any overlap) is not an accident :)

Anyway, can't argue against your argument, and I'm not in the mindset of cutting pixels in half :)
So, should i remove size.padding.buttontable, and just go with button=2?

I guess your touch area extension might be tweaked around:

self.dimen = self.frame:getSize()
self[1] = self.frame
if Device:isTouchDevice() then
self.ges_events = {
TapSelectButton = {
GestureRange:new{
ges = "tap",
range = self.dimen,
},

enlarging range to be a little bigger than self.dimen (except when provided with hints by buttontable on which side(s) it shouldn't, for buttons that are bordered by other buttons.
(I'm not much interested in working on that, but may be you would? :)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

Yes, but when we have a button table like we do, with stuck buttons without any margin, the fact that you can't extend the tap area without any overlap/conflict (which has precedence? these shouldn't be any overlap) is not an accident :)

Ah, now I see what you mean by "overlap." I thought you meant it'd be difficult to touch the middle button (and I didn't quite follow but I just shrugged it off ;-) ). There's nothing fancy going on there. There's just an outside margin that belongs to the button and thus to its tap area. It's split perfectly down the middle.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

No, I mean if you have a 3x3 table of buttons without any visual margin, with each a height of 36:
you can extend the tap area of those on the borders (in 1 or 2 directions), but not in the directions of those that are bordered, and not at all for the one in the middle, which just can't have a tap area taller than 36.
So, taping on those in the middle will require the same precautions as previously - while taping on those on the sides, with their larger tap area, will be easier, and avoid accidental dismiss.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

There's just an outside margin that belongs to the button and thus to its tap area.

(Wording another way my previous comment:) ^ except for the middle buttons that can't get that tap'able outside margin.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

But every button has the exact same margin. I'm afraid I'm not following. And in any case the specific margin in the screenshot is irrelevant. My point is margin > 0.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

But every button has the exact same margin

Yes, 0-margin (visual margin) in our ButtonTable case.
But if you can add some tapable non-visual margin, you can only add it on the buttons on the sides :)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Illustration :)

image

In yellow, that non-visual extended tapable margin.
In blue some buttons with an extended tapable height.
In red some buttons which can't have their tap'able area extended.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Anyway, can't argue against your argument

Although I could :) That same page with your section about touch targets (for standalone elements/buttons) shows tons of menu/lists with a huge vertical empty space separator (like on your ios vs "bad" android screenshot).
So, if we rename ButtonTable to ItemsGrid, and forget about the idea of buttons, I could use Size.padding.itemsgrid= 4, and you won't notice :)

Anyway, if we revert this PR, it's a single line change (if we drop the horizontal 0-padding and use the default button on the left/right sides too):

--- a/frontend/ui/widget/buttontable.lua
+++ b/frontend/ui/widget/buttontable.lua
@@ -61,3 +61,3 @@ function ButtonTable:init()
                 margin = 0,
-                padding = 0,
+                padding = Size.padding.button,
                 text_font_face = self.button_font_face,
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

Although I could :) That same page with your section about touch targets (for standalone elements/buttons) shows tons of menu/lists with a huge vertical empty space separator (like on your ios vs "bad" android screenshot).

Well of course it would. Android is "material design" after all. :-P I was only using the material design screenshot because it illustrates the principle of using margin and/or extended touch space much better than I wanted to put time into. I think material design isn't very good looking and hard to use.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

I discovered I don't hate the extra whitespace if it's actual whitespace.

At 4


At 10


But perhaps it doesn't even have to be gone. It also helps to change the separator line to thin (although not as much).

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Well, the absence of separator is quite a bit more of a visual change that what I aimed for :)
It looks nice on the search bar, but I'd feel a bit lost/shy in the multi lines grids, even the dict window.
But I'd have to try it to see how it feels on a device (might be a cheap way to get KOReader a new modern look :)
I guess I'd be fine with a thin line :) (I thought it was already thin)

Can you share some patch?

("At 10" means Size.padding.buttontable = Screen:scaleBySize(10) ?)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 1, 2019

Oh, there's nothing to that. Either comment 'em out or just give the separator a width of 0.

diff --git a/frontend/ui/widget/buttontable.lua b/frontend/ui/widget/buttontable.lua
index 676a1661..639e9244 100644
--- a/frontend/ui/widget/buttontable.lua
+++ b/frontend/ui/widget/buttontable.lua
@@ -18,9 +18,8 @@ local ButtonTable = FocusManager:new{
             {text="Cancel", enabled=false, callback=nil},
         },
     },
-    sep_width = Size.line.medium,
+    sep_width = 0,
     padding = Size.padding.default,
-
     zero_sep = false,
     button_font_face = "cfont",
     button_font_size = 20,
@@ -59,8 +58,8 @@ function ButtonTable:init()
                 max_width = (self.width - sizer_space)/column_cnt - 2*self.sep_width - 2*self.padding,
                 bordersize = 0,
                 margin = 0,
-                padding = Size.padding.buttontable, -- a bit taller than standalone buttons, for easier tap
-                padding_h = 0, -- allow text to take more of the horizontal space
+                padding = Screen:scaleBySize(10), -- a bit taller than standalone buttons, for easier tap
+                padding_h = Screen:scaleBySize(2), -- allow text to take more of the horizontal space
                 text_font_face = self.button_font_face,
                 text_font_size = self.button_font_size,
                 show_parent = self.show_parent,
@@ -79,12 +78,12 @@ function ButtonTable:init()
             buttons_layout_line[j] = button
             table.insert(horizontal_group, button)
             if j < column_cnt then
-                table.insert(horizontal_group, vertical_sep)
+                --~ table.insert(horizontal_group, vertical_sep)
             end
         end -- end for each button
         table.insert(self.container, horizontal_group)
         if i < row_cnt then
-            self:addHorizontalSep(true, true, true)
+            --~ self:addHorizontalSep(true, true, true)
         end
         if column_cnt > 0 then
             --Only add line that are not separator to the focusmanager
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Nov 1, 2019

Well, I quite like it :) With 10 and no separator, it's quite pretty in the onHold dialogs (text selextion, file browser) - and nicer than before when you hold on it to make it translucide.
The only ones I don't find prettier are the dict window (too much white at bottom) and the View HTML (also too much white at bottom, and hard to feel these are individual buttons).
Also, it may look nice in english with small texts, might be less nice in languages with larger wordings/truncated with ellipsis text.

But as soon you add a thin separator, 10 is too large and 4 gets nicer.

Dunno if we'll get other cheap ideas like that, but that kind of change could be enabled with some checkbox Screen > [x] Modern UI :)

(Actually, for the original PR, if we go with padding.button=2 , the use of 4 could be made optional with a hidden setting, for people who really have a hard time with small button tables - or have a [x] Accessibility to enable larger/uglier things.

Frenzie added a commit to Frenzie/koreader that referenced this pull request Nov 10, 2019
In preparation for 2019.11 this commit improves the visual style
without completely overhauling it yet.

Cf. koreader#5554 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.