New issue
Advanced search Search tips

Issue 893535 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Dynamic Type: The ReadingList background text should be fully visible with large font

Project Member Reported by gambard@chromium.org, Oct 9

Issue description

When the user select the biggest preferred content size category, the text displayed when the Reading List is empty isn't fully visible: some is displayed below the top and bottom toolbars.

sczs@, kkhorimoto@: Do you have ideas about how to do this?
 
Simulator Screen Shot - iPhone X - 2018-10-09 at 13.28.12.png
169 KB View Download
Can we set a "max" supported dynamic font size? I think that would be the best solution. 
We can also try changing the padding on the left and right, which should help fit the whole text, but it will look different from the mock on the "default" font.

Yeah I think we've got a couple options:
1) Set a limit to the max font size supported.  This would require doing some text layout calculations to calculate what the max supported size would be though.  We could use ManualTextFramer for that, but it seems like it might become overly complex.
2) Make the empty table view background scrollable.  This would be complicated in its own way, as embedding scroll views within each other can be tricky.  I think we could probably accomplish this by adding a dependency between the background scroll view's pan gesture and the table view's pan gesture, such that the table view's gesture requires the background view's gesture to fail.

I'm not really sure what the better option would be between these two.  It's probably something we should get some design input on, as I can't really tell whether it's better to limit our dynamic type support or have giant scrollable text that only fits 2-3 words per line.
I usually don't like setting a max text height because it is adding lot of hard to maintain code and you don't really know how small it can end up with translations.

I asked Martijn who thinks that it is better to have something scrollable.

I though about adding it in a cell instead of a background. Like that we know that it is scrollable. WDYT?
Yeah using a cell might be an easier option here, since the scrolling behavior is already built in to the view.  It might become more complicated to implement though, as we'd need to have a separate mode for the datasource to support this.  Using a scroll view background view seems closer to our current approach in terms of setting up the UI with respect to the table view model code.  It's also unclear if we'd want the background view to be scrollable if it fits entirely on the screen; table view cells will still be scrollable, but a separate scroll view would have scrolling disabled if the contentSize is less than the view size.
I agree with Kurt on not adding it as a cell. We're sharing this with all the other lists and adding this a cell would break the internal bookeeping/model for the table content, and we wouldn't be able to rely on the tableView being empty for the business logic. We also don't want to start fragmenting the implementation of our lists. 

Did you discuss the option of changing the side margins/padding + setting a max translation size in the .grd file? I think that might be enough and it would be easy to implement.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe100b299e7bc889ccafc03cf8854efe64020679

commit fe100b299e7bc889ccafc03cf8854efe64020679
Author: Gauthier Ambard <gambard@chromium.org>
Date: Fri Oct 12 09:41:43 2018

[iOS] Support Dynamic Type for ReadingList empty string

This CL changes the string displayed when the ReadingList is empty so
it doesn't overflow below the bottom and top toolbar when the font is
too big. It does that by limiting the font size.

Bug:  893535 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I03335f2c1fee192a765b6e85011627c0f5ae2019
Reviewed-on: https://chromium-review.googlesource.com/c/1276925
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599153}
[modify] https://crrev.com/fe100b299e7bc889ccafc03cf8854efe64020679/ios/chrome/browser/ui/reading_list/empty_reading_list_message_util.mm
[modify] https://crrev.com/fe100b299e7bc889ccafc03cf8854efe64020679/ios/chrome/browser/ui/reading_list/reading_list_table_view_controller.mm

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38ea2d27a74e33acdb97c0ced210be48ee34a473

commit 38ea2d27a74e33acdb97c0ced210be48ee34a473
Author: Gauthier Ambard <gambard@chromium.org>
Date: Mon Dec 03 15:11:42 2018

[iOS] Make the TableView's empty view scrollable

This CL adds a scroll view containing the text displayed when the table
view is empty. It allows the user to scroll the text if the font size
used is big.
As the safe area insets aren't propagated to the empty view, this is
using dummy view to fake the safe area insets in the scroll view.

Bug:  893535 
Change-Id: I63b658c1bc9d5cacdb4e86d201a2bb2ca7621a96
Reviewed-on: https://chromium-review.googlesource.com/c/1353941
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613087}
[modify] https://crrev.com/38ea2d27a74e33acdb97c0ced210be48ee34a473/ios/chrome/browser/ui/table_view/chrome_table_view_controller.mm
[modify] https://crrev.com/38ea2d27a74e33acdb97c0ced210be48ee34a473/ios/chrome/browser/ui/table_view/table_view_empty_view.h
[modify] https://crrev.com/38ea2d27a74e33acdb97c0ced210be48ee34a473/ios/chrome/browser/ui/table_view/table_view_empty_view.mm

Sign in to add a comment