Opened 4 years ago

Closed 3 years ago

#3097 closed defect (fixed)

Supplemental filter list of a filter list which is already a supplemental list doesn't appear on subscriptions page

Reported by: arthur Assignee: juliandoucette
Priority: P2 Milestone:
Module: Websites Keywords:
Cc: greiner, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29337807/

Description

How to reproduce

  1. Go to https://adblockplus.org/subscriptions
  2. See there is no supplemental list right below "EasyList China"

Observed behaviour

There is no supplemental list right below "EasyList China".

Expected behaviour

"CJX's Annoyance List" which I added 2 days ago should appear below "EasyList China". EasyList China is already a supplemental list for EasyList so that's likely the problem here.

Change History (15)

comment:1 Changed 4 years ago by saroyanm

  • Priority changed from Unknown to P3
  • Ready set

comment:2 Changed 4 years ago by saroyanm

  • Priority changed from P3 to P2

comment:3 Changed 4 years ago by juliandoucette

To be clear:

Are we proposing that we allow another layer of depth to this table by allowing a subscription row that supplements to be supplemented?

comment:4 Changed 4 years ago by saroyanm

I think we should generate the content regardless of the depth.

comment:5 Changed 4 years ago by juliandoucette

  • Owner set to juliandoucette

comment:6 follow-up: Changed 4 years ago by juliandoucette

I don't see "CJX's Annoyance List" in the subscriptions list anymore.

Is this still a relevant issue Arthur?

comment:7 Changed 4 years ago by juliandoucette

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 in reply to: ↑ 6 Changed 4 years ago by arthur

Replying to juliandoucette:

I don't see "CJX's Annoyance List" in the subscriptions list anymore.

Is this still a relevant issue Arthur?

It's still in the repository, so I would say it's still relevant: https://hg.adblockplus.org/subscriptionlist/file/tip/CJX's%20Annoyance%20List.subscription

comment:9 Changed 4 years ago by juliandoucette

My mistake, the template code was excluding it from the list. I will upload a new solution that supports multiple levels soon :) .

comment:10 Changed 4 years ago by juliandoucette

The subscriptions table currently uses a "dummy" column to offset supplemented lists. This works ok (although it is not accessible) for one level of depth. However, it does not scale because it is not possible to set a different column widths per row. I have come up with a couple solutions for this.

Solution 1

Create 10-20 columns via colgroup on the table. Then manually set the colspan of each column. EG:

table with 10 columns

  • row 1
    • col 1 spans 1 (dummy)
    • col 2 spans 4
    • col 3 spans 5
  • row 2
    • col 1 spans 2 (dummy)
    • col 2 spans 3
    • col 3 spans 5

This works, but it is limited by the number of columns. And, even if we add accessible text to the dummy column, it's not easy to navigate for screen readers.

Solution 2

  1. Remove the dummy Column.
  2. Set the first column's position to relative and remove its border.
  3. Create an inner div that has an absolute position of top = 0, left = offset, width = 100% - offset, height = 100% and border.

This works. And it is not limited. But even if we add accessible text to the first column, it's not easy to navigate for screen readers because there is no clear parent/child relationship in the markup.

Solution 3

My recommendation / the one I am going to implement unless someone objects.

Replace the tables with unordered lists. Use lists inside lists for supplemented lists.

This works. It is not limited. And it is accessible. I believe that an unordered list is the appropriate element for this use case. It will take more time/effort to style and more/time effort to review because it is a slightly bigger change.

comment:11 Changed 4 years ago by juliandoucette

  • Review URL(s) modified (diff)
  • Status changed from reviewing to reopened

comment:12 Changed 4 years ago by greiner

No objections from my side. Replacing the table with nested lists appears to be the most sane solution.

comment:13 Changed 4 years ago by juliandoucette

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:15 Changed 3 years ago by juliandoucette

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.