Opened 3 years ago

Last modified 12 days ago

#2708 new change

Implement lazy loading for custom filters in new options page

Reported by: saroyanm Assignee: agiammarchi
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner Blocked By: #2376
Blocking: Platform: Unknown
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

With #2376 we are going to implement custom filters list in Advanced tab of new options page, but we still need to implement lazy loading for that list, to make the page load more efficient.

What to change

Implement lazy loading for custom filters list in advanced tab for new options page.

Attachments (2)

infinite.html (6.5 KB) - added by agiammarchi 3 weeks ago.
infinite area sketch
filters-table.zip (4.6 KB) - added by agiammarchi 3 weeks ago.
Closer to original XUL + a11y improvements

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by greiner

  • Priority changed from Unknown to P3

comment:2 follow-up: Changed 4 weeks ago by agiammarchi

  • Tester set to Unknown

As far as I can tell we are already loading filters asynchronously.

      for (let subscription of subscriptions)
      {
        browser.runtime.sendMessage({
          type: "filters.get",
          subscriptionUrl: subscription.url
        },
        (filters) =>
        {
          loadCustomFilters(filters);
        });
      }

maybe this ticket simply got forgotten ? All I can do here is probably just change the code as such:

      for (let subscription of subscriptions)
      {
        browser.runtime.sendMessage({
          type: "filters.get",
          subscriptionUrl: subscription.url
        },
        loadCustomFilters);
      }

but I don't see much gain in doing so. Please let me know what to do here, thanks.

comment:3 in reply to: ↑ 2 Changed 4 weeks ago by saroyanm

Replying to agiammarchi:

As far as I can tell we are already loading filters asynchronously.

      for (let subscription of subscriptions)
      {
        browser.runtime.sendMessage({
          type: "filters.get",
          subscriptionUrl: subscription.url
        },
        (filters) =>
        {
          loadCustomFilters(filters);
        });
      }

maybe this ticket simply got forgotten ? All I can do here is probably just change the code as such:

      for (let subscription of subscriptions)
      {
        browser.runtime.sendMessage({
          type: "filters.get",
          subscriptionUrl: subscription.url
        },
        loadCustomFilters);
      }

but I don't see much gain in doing so. Please let me know what to do here, thanks.

Some people might have a lot of filters, I'm not sure how well textarea(Though when this ticket was created we were using different element) is handling a lot of filters ex. 50 000 and more..
But that probably will depend on the browser, so this is about populating filters in the view on scroll, but probably make sense to test if it's currently an issue first, let's say for ex. by loading EasyList into the textArea.

comment:4 Changed 3 weeks ago by agiammarchi

EasyList is indeed impossible to handle with just a textarea, even if no javascript is involved.

I'm working on something smarter than a textarea, something that works with as many rows as we like, something that edits one row per time instead of storing all info at once, something that could bring inline filter validation, instead of multiline validation per each change.

Please have a look at this infinite area: https://drive.google.com/file/d/1loFpRKUZsLLGHzKpMCRnj2byFpLDyysU/view?usp=sharing and let me know what do you think about it.

It requires the replacement of the current textarea and quite some extra work on top but it would provide line per line validation on editing, not just as a whole, and with more accurate results thanks to the ability to pass along the incriminated edited line right away instead of working with a list of text lines split by \n.

Looking forward to your opinions.

comment:5 Changed 3 weeks ago by greiner

It looks quite impressive and seems to cover all use-cases we may want to tackle in the future so I like it. Is it based on a preexisting solution or is it a custom implementation?

Have you discovered any limitations with this approach so far that would need to be ironed out? (e.g. odd behaviors when scrolling through the list quickly)

comment:6 follow-up: Changed 3 weeks ago by agiammarchi

Is it based on a preexisting solution or is it a custom implementation?

I've written various infinite-scrolling things here and there so I'd lie saying it came out of the blue but it's 100% custom, also because it's the first time I use common infinite scrolling patterns to simulate a textarea (previously: table rows, "todo"/item lists, etc)

the contenteditable attribute does basically everything else: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable

that means Aria and a11y related issues should be simplified by the browser right away.

Have you discovered any limitations with this approach so far that would need to be ironed out?

There is surely something to improve there, it's rather a sketch, and I've attached it to this ticket. Scrolling quickly is fine, the area is small enough to have zero performance issues, but you cannot select all the text at once or paste multiple lines at once.

We need some logic to handle these use cases because with something that looks like a textarea, everything common with textareas will be used and it's expected to work.

If we could have some proper spec based on this prototype that rules out most common use cases to deal with, I could estimate how much effort it could take to adapt current sketch or if possible at all.

We target modern browsers thought, so I expect to have not many issues.

Changed 3 weeks ago by agiammarchi

infinite area sketch

comment:7 in reply to: ↑ 6 Changed 3 weeks ago by greiner

Replying to agiammarchi:

There is surely something to improve there, it's rather a sketch, and I've attached it to this ticket. Scrolling quickly is fine, the area is small enough to have zero performance issues, but you cannot select all the text at once or paste multiple lines at once.

We need some logic to handle these use cases because with something that looks like a textarea, everything common with textareas will be used and it's expected to work.

If we could have some proper spec based on this prototype that rules out most common use cases to deal with, I could estimate how much effort it could take to adapt current sketch or if possible at all.

We don't have to emulate a text area so feel free to create a spec ticket to suggest replacing it with an editable list so that we can adapt the UI to what we can already do with this approach instead of the other way around.

The reason why we've been using a text area in the options page is simply to achieve feature parity with the old Chrome options page which is using <select multiple>.

comment:8 Changed 3 weeks ago by agiammarchi

  • Owner set to agiammarchi

Changed 3 weeks ago by agiammarchi

Closer to original XUL + a11y improvements

comment:9 Changed 12 days ago by agiammarchi

This is the current specification proposal with live example:
https://gitlab.com/eyeo/spec/issues/136

Note: See TracTickets for help on using tickets.