Skip to content

luci-base: Make some tables better searchable#8191

Closed
Ramon00 wants to merge 1 commit intoopenwrt:masterfrom
Ramon00:makesearchable
Closed

luci-base: Make some tables better searchable#8191
Ramon00 wants to merge 1 commit intoopenwrt:masterfrom
Ramon00:makesearchable

Conversation

@Ramon00
Copy link
Copy Markdown
Contributor

@Ramon00 Ramon00 commented Jan 4, 2026

This changes the "name" in specific tables from being painted to being rendered. This allows the use of "ctrl-f" in browsers to e.g. search for firewall rule names.

Can anybody test this out? If no issues, I will then also change the other themes.

P.S. Any reason why there is 2nd section in the css file:

.cbi-section-table-titles.named::before,
.cbi-section-table-descr.named::before,
.cbi-section-table-row[data-title]::before {

Maybe just remove this as well?

@github-actions

This comment has been minimized.

@systemcrash
Copy link
Copy Markdown
Contributor

Is there a quick litmus test that I can use to verify the changes? I use FF and search works fine there.

@vincejv
Copy link
Copy Markdown

vincejv commented Jan 5, 2026

@systemcrash the issue is reproducible on Chromium based browsers

Steps to reproduce

  1. Go to Luci > Network Firewall > Traffic Rules
  2. Press CTRL+F to bring up browser search functionality
  3. Enter search term that can be found in the page
  4. Does not return anything as reported by browser as seen in the screenshot
image

@systemcrash
Copy link
Copy Markdown
Contributor

Thanks @vincejv - I'm aware. I don't run Chrome anything, which is why I ask for another way to verify...

@jow-
Copy link
Copy Markdown
Contributor

jow- commented Jan 5, 2026

Please test on a mobile screen after this change, that content approach was used to be able to dynamically hide/re-position row titles to headlines when decomposing the table into cards.

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 5, 2026

Thanks @vincejv - I'm aware. I don't run Chrome anything, which is why I ask for another way to verify...

I think the progession part is sufficiently tested. I think testing regression impact is more important, i.e. is the visual layout affected? So Firefox test would be great to do as well.

Please test on a mobile screen after this change, that content approach was used to be able to dynamically hide/re-position row titles to headlines when decomposing the table into cards.

If i view it portrait on mobile its pretty identical (other than a grey background bar missing in e.g. the rule name).

Oh also the text is not in bold anymore (mobile or desktop), its same style as the other text, if needed I can make it bold again, if people prefer that. (The bold was not that functional I thought, but im fine either way). Just let me know what people prefer.

@vincejv
Copy link
Copy Markdown

vincejv commented Jan 7, 2026

@systemcrash

I use FF and search works fine there.
which is why I ask for another way to verify...

it doesn't work on Safari either, tested on iPhone with iOS 26.2, now if someone can do the same test on Mac.

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 10, 2026

@systemcrash

I use FF and search works fine there.
which is why I ask for another way to verify...

it doesn't work on Safari either, tested on iPhone with iOS 26.2, now if someone can do the same test on Mac.

Chromium based browsers suffer from this "feature" (=not searching painted items). This is a design choice as those ::before elements should only contain visual things, not relevant information. We can question if this is the right choice in the browser or not, but regardless, chromium based browsers suffer from this. I think the market share is enough of chromium based browsers to warrant fixing this.

What I do think needs maybe more testing is if on all browsers the patch does not mess up the layout anywhere (we dont want to substitute one issue for another).

@vincejv
Copy link
Copy Markdown

vincejv commented Jan 10, 2026

safari is not chromium based though. However chromium based browsers uses blink engine which was forked from WebKit.

this design decision could stem back from WebKit. So if we fix this upstream, we have to file 2 separate issues for both WebKit/Safari and Chromium. However, since search only “works” correctly in Firefox, I agree with you that it makes sense to follow the behavior of the two engines with much larger user bases.

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 10, 2026

I agree
(the point i was trying to make, that i dont think it makes sense to do a full inventory of which browser has the issue and which browser does not, before fixing it)

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 16, 2026

Updated all themes now.

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 16, 2026

This is what it will look now:

Bootstrap
image

Material
image

Openwrt
image

Openwrt-2020
image

@systemcrash
Copy link
Copy Markdown
Contributor

What happens on mobile? :)

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 18, 2026

Looks fine to me:

image

@jow-
Copy link
Copy Markdown
Contributor

jow- commented Jan 18, 2026

The label text ("printer port") should be slightly bigger and bold afair

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 18, 2026

The label text ("printer port") should be slightly bigger and bold afair

Currently the font is not bigger, the only delta is it being bold.
But i can make it bigger and bold by reusing a css item or making a new one. To be honest I do not think the bold or bigger is very functional, it does not make the pages read easier or be better to understand, but im fine with adding that.

How big do you want it to be?

@systemcrash
Copy link
Copy Markdown
Contributor

font-weight: bold; should probably be retained, then.

This changes the "name" in specific tables from being painted to
being rendered. This allows the use of "ctrl-f" in browsers to e.g.
search for firewall rule names.

Signed-off-by: Ramon Van Gorkom <Ramon00c00@gmail.com>
@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 18, 2026

image

How about that?

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 18, 2026

I can add something like font-size: 110%; if that makes it look better

trEl.appendChild(E('th', {
'class': 'th cbi-section-table-cell',
'data-sortable-row': this.sortable ? '' : null
}, (!this.anonymous || this.sectiontitle) ? _('Name') : null
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

						},	[ (!this.anonymous || this.sectiontitle) ? _('Name') : null ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, i dont think the square brackets are needed here, we only add a single element not an array. Looks like appendChild also only supports a single element. Maybe it is actually better to remove them in line 2580 as well?

I did do a quick search in the luci code base and both are used. So both seem to work. Let me know if you want me to add it here or remove on 2580

systemcrash pushed a commit that referenced this pull request Jan 23, 2026
This changes the "name" in specific tables from being painted to
being rendered. This allows the use of "ctrl-f" in browsers to e.g.
search for firewall rule names.

Closes #7708
Signed-off-by: Ramon Van Gorkom <Ramon00c00@gmail.com>
Link: #8191
Signed-off-by: Paul Donald <newtwen+github@gmail.com>
(cherry picked from commit 720c96c)
systemcrash pushed a commit that referenced this pull request Jan 23, 2026
This changes the "name" in specific tables from being painted to
being rendered. This allows the use of "ctrl-f" in browsers to e.g.
search for firewall rule names.

Closes #7708
Signed-off-by: Ramon Van Gorkom <Ramon00c00@gmail.com>
Link: #8191
Signed-off-by: Paul Donald <newtwen+github@gmail.com>
@systemcrash
Copy link
Copy Markdown
Contributor

Merged via 720c96c. Thanks @Ramon00 !

@eamonxg
Copy link
Copy Markdown
Contributor

eamonxg commented Jan 24, 2026

Hi there ,

First of all, thank you for your work on updating LuCI!
I really appreciate the improvements in the new version. I wanted to bring up a small backward compatibility issue I noticed regarding the table first column, which might help improve theme compatibility.

In previous versions of LuCI, the first column of tables was displayed using
.tr[data-title]::before and .td[data-title]::before (with data-title).
In a recent update, this implementation was changed to use actual DOM elements instead.

Currently, themes handle this change by simply disabling the ::before pseudo-element in CSS (for example, using content: none).
While this works fine with the new version, there is a potential compatibility issue:

If a user is still on an older version of LuCI that relies on ::before for the first column, but installs the latest theme, the first column disappears because ::before is directly disabled.
Simply overriding the pseudo-element in CSS can unintentionally break the display for older LuCI versions.

Illustration of the Issue

Scenario Outcome Screenshot / Illustration
Old LuCI (uses ::before) + old theme ✅ First column displays correctly has-before
Old LuCI (uses ::before) + new theme ❌ First column disappears no-before

@eamonxg
Copy link
Copy Markdown
Contributor

eamonxg commented Jan 24, 2026

Hi @Ramon00 @systemcrash ,

I have a suggestion that might help maintain backward compatibility:

One idea is to add a simple flag to indicate that ::before is no longer used in the new implementation, for example:

<td data-title="xxx" data-no-before></td>
<tr data-title="xxx" data-no-before></tr>

With this flag, themes could use a selector to conditionally apply the old ::before styles, enabling backward compatibility, for example:

.td[data-title]:not([data-no-before])::before {
  /* original styles */
}

.tr[data-title]:not([data-no-before])::before {
  /* original styles */
}

This would allow older versions of LuCI that still rely on ::before to continue displaying the first column correctly, while the new implementation remains unaffected.

@systemcrash
Copy link
Copy Markdown
Contributor

Hmm - thanks for the information @eamonxg. I won't pick this to previous versions - only to 25.12. Those are some good suggestions.

@eamonxg
Copy link
Copy Markdown
Contributor

eamonxg commented Jan 24, 2026

Hmm - thanks for the information @eamonxg. I won't pick this to previous versions - only to 25.12. Those are some good suggestions.

Thanks for the reply, I hope this could be considered, as third-party themes need to support both versions before 25.12 and the latest version. A simple data-no-before flag would make handling this much easier without affecting the new implementation.

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 24, 2026

Yeah indeed you can't just update or install one of the two halfs. Adding style data into the js code makes updating in the future more complicated. I guess just using this on 25.12 is probably a good compromise, as it only affects the test releases where you can expect some "bugs" and only have limited audience, while still keeping the code not too complicated.

@eamonxg
Copy link
Copy Markdown
Contributor

eamonxg commented Jan 24, 2026

Adding style data into the js code makes updating in the future more complicated

Since the goal is to avoid making the code more complicated, what about completely removing the named and data-title logic that was used for displaying the ::before pseudo-element?
image

@Ramon00
Copy link
Copy Markdown
Contributor Author

Ramon00 commented Jan 24, 2026

what about completely removing the named and data-title logic that was used

I am all for removing obsolete code. The thing is, you have to be careful not to break something if that data-title tag was still used. Leaving it in may also make debugging easier. Removing those tags will of course not fix the issue of installing a later version of a theme with an old version of the luci code. Regardless the issue will be gone once rc4 gets build (i guess rc3 got tagged just two days too early, maybe for some of the builds its already ok...)

@systemcrash
Copy link
Copy Markdown
Contributor

You can still image build in a few days a get the new theme.

eamonxg added a commit to eamonxg/luci-theme-aurora that referenced this pull request Jan 29, 2026
Following upstream LuCI commit 720c96c, the first column is now
explicitly handled as a field rather than a pseudo-element.

This prevents layout duplication and ensures compatibility with the
new table rendering logic.

Ref: openwrt/luci#8191
@Ramon00 Ramon00 deleted the makesearchable branch March 18, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants