mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.23k stars 2.93k forks source link

History List Entries don't populate date buckets correctly #15888

Open jjSDET opened 1 year ago

jjSDET commented 1 year ago

During performance testing of the History List, I ran into a number of issues with how the history items are displayed.

I found six use cases where the dated history items weren't handled correctly and displayed in their correct date bucket.

Testing was difficult so I extended our current tool, ./test-fixtures/generate-test-db.py that modifies our Simulator dbs to also add dated history items to today, yesterday, a week, a month, and older for use with testing this bug.

The History List, unlike the Bookmarks List, throttles the max element loading to 100 items when you visit the page. This requires scrolling through the items before they will populate the various date buckets, and this delayed processing resulted in a number of issues.

  1. When 100 entries are in each date bucket ‘today’, ‘yesterday’, ‘a week’, ‘a month’, and ‘older’, only ‘today’ is visible when going to the History List.
  2. When 50 entries are in each bucket, only ‘Today’ is loaded when visiting the History List, as well as, when you scroll through ‘Today’, instead of seeing all of the history buckets, the only ones displayed are ‘Today’, ‘Last Week’, and ‘Older’, even though you can verify the entries are in the database.
  3. When seeing Bug #2, additionally when you navigate out of the History List and back to the page and scroll through all of the ‘Today’ entries, all categories will intermittently show up; however, not all entries are displayed where they should be. In this example, ‘Yesterday’ now has only 3 entries, even though there should be 50. Screenshot 2023-07-27 at 11 54 30 AM
  4. Intermittently, I’ve found when I scroll quickly through ‘Today’, some entries don’t become populated in the date buckets.
  5. This is likely a symptom of the same issue, but I’ve also noticed if i scroll through all of the entries for ‘Today’ through ‘Yesterday’ and ‘A week’, I have to scroll through ‘Today’ to continue populating the list of entries instead of being able to continue scrolling through the last date bucket.
  6. When there are a large number of entries, like 500, though it could be as small as 200, sometimes the next date bucket won’t load until navigating away from the History List and back to it. Even then, not all entries load for the next date bucket. If you scroll too quickly, you will reach the bottom of the date bucket and can’t scroll anymore, even though there are 2000 entries left to load. If you navigate away from the page and back and scroll slowly, the next date buckets will sometimes load.

Steps to reproduce

Modify a performance test to run a test with your own db. For example, I created a new test ‘testHistoryDatabaseFixtureJJ’, and added a breakpoint on the assertion verifying the element count. Screenshot 2023-07-27 at 12 13 36 PM Then I modify the ‘fixtures’ array with my db i’ll be using for testing mapping my new test to the test db used when running the test, name it anything you’d like: Screenshot 2023-07-27 at 12 14 13 PM Then I ran the script in ‘./test-fixtures’, ‘generate-test-db.py’, passing in the arguments i wanted to test. You can also run it without arguments to be prompted for what you need. Screenshot 2023-07-27 at 12 16 01 PM For example, from the root directory of Client you can use python3 ./test-fixtures/generate-test-db.py -history 500 -bookmarks 0 -today -yesterday -week -month -older -db_name jj-places -bundle_identifier org.mozilla.ios.FennecEnterprise You can add or remove any of the date buckets if you just wanted to test, let’s say, ‘week’ and ‘older’, none of them are required, though the script will default to ‘today’ if no date bucket is specified.

PLEASE NOTE: the database connection sometimes doesn’t release because of weirdness with iOS Simulator. I couldn’t figure out a 100% way to address that; HOWEVER! If you make sure you finish running the test until it fails, then closing the simulator and reopening the simulator before your next test run, it seems to work 95% of the time.

What I did was change the assertion TIME_OUT to 3 seconds, then continued the test by clicking the ‘play/continue program execution’ button on the debugger console. Screenshot 2023-07-27 at 12 27 53 PM Note the change on line 69 to 3 seconds timeout. The ‘continue program execution button’ is the one to the right of the blue flag symbol on the bottom left.

Expected behavior

The dated history entries should populate in the correct date buckets.

Actual behavior

Only the first 100 are listed in the earliest date bucket with entries and the rest don't populate correctly when scrolling through the entries as they load.

Device & build information

┆Issue is synchronized with this Jira Spike

data-sync-user commented 1 year ago

➤ Daniela Arcese commented:

Let’s investigate what is happening here and propose possible solutions.

razvanlitianu commented 3 months ago

@jjSDET it's been while since this bug was reported. Is it still reproducible?

razvanlitianu commented 3 months ago

I can reproduce the issue described in the bug report, but debugging is difficult with only the testing environment. I would like to set up a mock database for testing purposes.

Additionally, I reviewed the history panel code, but I'm unclear on how fetching and prefetching work. There is a queryFetchLimit parameter, which is currently set to 100. Changing this to 50 makes the no2 work correctly.

In my tests, setting the limit to 10 results in only one item being fetched, even though I have 10 items in the history. Increasing the limit to 20 still fetches only one item. It only works as expected when the limit is set to a higher value.

razvanlitianu commented 3 months ago
  1. is a side effect of prefetching. We only receive the data we've requested, so we won't be able to load additional data until we've moved past the 'Today' category. This isn't ideal, but it's unclear how it should function from a UX standpoint. Loading some data from each category isn't a good solution either because we need all the data.
data-sync-user commented 3 months ago

➤ Razvan Litianu commented:

Norberto Andres Furlan