luci-theme-footstrap: fixes from the field and from review - #8981
Open
VizzleTF wants to merge 6 commits into
Open
luci-theme-footstrap: fixes from the field and from review#8981VizzleTF wants to merge 6 commits into
VizzleTF wants to merge 6 commits into
Conversation
BKPepe
suggested changes
Aug 25, 2026
BKPepe
left a comment
BKPepe
left a comment
Member
There was a problem hiding this comment.
I appreciate that you split the changes up instead of putting everything into one large commit like in #8978, but I think this goes a bit too far in the other direction.
Twelve commits for what is essentially one theme update feels overly granular. I'd prefer to see related changes squashed into a few logical commits, rather than having a separate commit for every small fix.
I'd say 3–5 commits would be a much better balance: enough separation to keep the history meaningful, but without making it unnecessarily fragmented.
VizzleTF
force-pushed
the
luci-theme-footstrap-sync
branch
from
August 25, 2026 15:20
643d8c9 to
1bf17fa
Compare
Contributor
|
One comment is about the verbose comments in footstrap in general. LuCI has js, lua & css minifying as default (defined in luci.mk, and small dedicated apps for the actions), so comments get stripped from the final binaries. So, your verbose comments get removed from your .js files , but I think that the ucode .ut comments ship along the compiled package, increasing the compiled size quite much. In some files, the comments are a clear majority of the content. They are good in documenting the reasoning for the code, but do increase the size. It is not that many kB, but probably a reason why this theme's compiled version is much larger than the other themes. Actually this theme's .apk file is as large as the four other themes together...
In the light of the generic size inflation during the years, not that much, but still something to think in future. What level of commenting, or how can we clean them off? |
openwrt-ai
reviewed
Aug 25, 2026
openwrt-ai
left a comment
openwrt-ai
left a comment
Collaborator
There was a problem hiding this comment.
Commit checks
42de08f"luci-theme-footstrap: keep the reader's place across a poll" — the message is entirely about scroll anchoring, but the commit also carries threecascade.csshunks that belong to the next commit and are described by its message: the.table.fs-stacked:has(.tr.table-titles) .td .cbi-progressbarpair (e697a41's "a meter's value sat on its own label once its column became a card") and.cbi-title-buttons/.cbi-section-remove(e697a41's "a page-title button row touched the heading above it, and the per-section Delete button touched the tab bar below it"). It additionally rewrites afs-overview.jscomment introduced one commit earlier (see inline).e697a41"luci-theme-footstrap: fix spacing and hiding on luci-base classes" — the message lists four spacing fixes, but the diff is 3 insertions / 2 deletions and contains only two of them (the.cbi-map > *gap and the.cbi-valuehairline) plus the[hidden]rule. The meter-in-a-card and title-button/Delete items it describes are in42de08f.38c414e"luci-theme-footstrap: fix the login page heading and the graph labels" — adds.cbi-section .table + h3{margin-top:var(--fs-space-5)}and.cbi-section h3 + .table{margin-top:var(--fs-space-3)}. Those are neither the login heading nor the graph labels, and no commit message in the series mentions them.
Since the split is already under discussion in #8981 (review by @BKPepe), it is worth landing the spacing hunks in the spacing commit while re-rolling — right now three of the five subjects do not describe their own diffs.
Generated by Claude Code
Contributor
Author
An 11-port switch drew five 200px cards per row in three rows, with 150px of empty column beside every one. The wrapper asked for repeat(auto-fit, minmax(126px, 200px)), and a grid takes its track count from the definite max, so the floor never decided anything. With minmax(var(--fs-port-min), 1fr) the count comes from the floor and 1fr shares the remainder: 11 ports on one row, and a short last row as wide as the rows above it. The floor is measured, not guessed. "1024.0 PiB" is the widest string luci.js's %1024.1mB can produce and those figures are nowrap, so a narrower card pushes its own numbers past its padding. Stepping a card 1px at a time until that fits gives 94px at Normal, 80 at Compact and 109 at Large, and the floor scales with the density axis. The card also stops repeating itself: the up/down dot is gone, since the speed line already reads "no link" or a rate while the coloured bar above it means the interface the port belongs to; the name is cut with an ellipsis and carries its full text as a native tooltip instead of setting every card's width; and the traffic figures always take a row of their own, so a linked port and a dark one read as the same layout. Their size is declared plainly in the rule and rounded in an @supports block, the way 02-tokens.css states the type ladder: an engine without round() drops the whole declaration, flag and all, and would inherit a size chosen for something else. Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
A poll refreshes a section with dom.content(), which empties the container
before it refills it. The document is briefly shorter than the offset the
reader sits at, the engine clamps it, and what happens next is the engine's
business: Chromium lands where it started, WebKit overshoots by 60px on
every
tick. On a live Safari that is the page creeping while you read.
The theme asked CSS.supports('overflow-anchor'), which answers "does this
engine anchor at all" -- a different question every current engine says yes
to. It measures instead: two frames after the mutation the reference it was
already holding is asked where it ended up, and whatever nobody put back is
given back. A synthetic probe was tried and rejected: it calls Firefox
broken,
because a real page puts layout and a frame between the collapse and the
refill.
Three faults sat behind the same symptom, all in which element the
reference
is taken on. elementFromPoint answers with #view in a gap between sections,
and the host's own top never moves; a point above the first section answers
with .fs-content, outside the host; and a page that is one table --
Processes,
Routes, the realtime lists -- has that table as a direct child of #view, so
the climb out of it landed on the host and gave up. The search now walks
the
element stack, steps down the viewport, refuses the host, anchors on the
table
where the climb would reach it, and falls back to the surviving section
when a
tick replaces the element it measured.
Two more things it had to learn. mousedown and keydown were routed
through the
handler that answers "the page is moving", so that answered yes for 400ms
after any click and every keystroke -- and it gates every pass that reads
layout: measured while typing with the window resizing under it, 9 of 10
passes were skipped and landed in one burst, against 0 of 10 now. And the
offset is brought forward after a correction while the reference is not,
since
the write moves the page by exactly the drift measured, leaving the
reference
where it was remembered.
216 runs across three engines, two releases, two layouts, two widths and
three
densities, with the engine's own anchoring on and off: no drift, no mid-
flick
surprise.
Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
Four things reported on one page each and fixed on the class rather than on
the page they were seen on:
* a block a view builds itself fused with the card below it. A view may
return a bare widget where a section is expected -- luci-app-irqbalance
does, for its /proc/interrupts snapshot -- and nothing gave that
block the
gap a section carries. The rule is written against what a map contains:
everything luci-base puts there and spaces itself is excluded, an empty
placeholder is left alone, and any other top-level block gets a card's
16px;
* a meter's value sat on its own label once its column became a card
(luci-mod-dashboard's Wireless list on a phone, 6-9px at every
density);
* a page-title button row touched the heading above it, and the per-
section
Delete button touched the tab bar below it -- every named section, so
SQM's queues and the firewall's zones alike;
* the hairline under a value row separates it from the NEXT row, but was
dropped only on :last-child, so a section that ends its rows and then
renders a table drew a line across the top of it, cutting through a
framed
table's rounded corner. Counted over eight stock pages, separators fall
only where one was dangling.
And one that is not spacing: el.hidden = true is the only way code that
ships
no stylesheet can hide an element, and the UA rule behind it loses to any
display a theme sets on a class. Measured here on .tr (table-row), .td
(table-cell), ul.nav > li (block), .cbi-page-actions (flex) and .ifacebox
(inline-flex), all of which painted a hidden element anyway. No stock page
trips it today, and the theme's own point fix for the same fault -- a
restated
display:none for the search overlay -- goes away with the general one.
hidden="until-found" is excluded, since display:none would break the
find-in-page reveal it exists for.
Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
The login page renders with no chrome, so the h1 header.ut prints never reaches it: the document went out with "Authorization Required" as an h2 and no h1 at all. Its card heading is the page title and is marked up as one, at the h2's size -- an h1's 26px wraps that string onto two lines in a 400px card, and the size was never what was wrong. The card also prints the hostname it belongs to, from the same `ubus system board` call and with the same escaping the chrome's wordmark and <title> already use. It discloses nothing new: that string has always reached an unauthenticated browser through <title>. Requested as openwrt#8961. Separately, every <text> that luci-mod-status's realtime .svg files draw for an axis carries an inline light grey with a one-pixel black halo, meant for the black background those files assume rather than for the panel a theme paints behind them: 1.16:1 on a light palette, where the numbers are legible only as the shadow around them. They take the theme's text colour now -- 17.9:1 light, 14.7:1 dark, where the halo is what has to go instead. The selector is a child combinator on purpose. Status -> Channel Analysis derives a colour per BSSID and paints both the curve and its SSID label with it, so there the inline fill is the only thing tying a label to its curve -- and those labels are built inside a <g>, while the axis labels are direct children of the <svg>. Verified by planting one label of each shape: the SSID label keeps its colour and halo, the axis label reads the theme's text colour without one. Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
Saving on System -> System redraws the map: the old tab group goes, a new one is built, and ui.tabs marks it ready by setting an attribute. The theme's observer watched childList only, so when that attribute landed after the last change to the group's children, the mount had already returned with no group to attach to and nothing woke it again -- the tab was missing until the next navigation. Reported on 25.12.5 from Chrome and from iOS as "sometimes it disappears after Save" (openwrt#8903). Modelled as a deterministic race: remove the pane, drop data-initialized, restore it 400ms later with no other mutation. Three failures out of three before, three passes out of three after, on 25.12 and 24.10. The observer now watches that attribute, and a miss retries on a widening delay, since the attribute alone would still rely on ui.tabs setting it that way. The sliders on that tab also drop a calcunits option they passed to ui.RangeSlider: the widget renders that span only when a calculate function produced a value, and no axis here passes one. Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
The traffic figures on a Port status card were sized with a literal in the page rule -- calc(10px * var(--fs-density-type)), stated twice, once plainly and once inside an @supports for round(). That was the only hardcoded font-size in the sheet: every other size resolves through the --fs-type-* ladder, and 10px sits one step below its smallest. --fs-type-2xs holds it now and is rounded in the same @supports block that rounds the rest of the ladder, so the page rule reads a token and the density axis stays in one place. The size is unchanged and it is a measurement: those figures are the widest thing a card must fit whole, and the floor goes 106px at --fs-type-xs to 94px here -- the difference between ten cards plus a lonely eleventh and one row of eleven. Signed-off-by: Ivan Kvashonkin <vizzlef@gmail.com>
VizzleTF
force-pushed
the
luci-theme-footstrap-sync
branch
from
August 25, 2026 16:41
1bf17fa to
a241def
Compare
Contributor
Do you have a ucode minification routine that could be imported here? Examples: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #8978. No 23.05 compatibility: that release is EOL, the theme has
dropped support for it, and the local
ui.RangeSlidercopy is gone(#8978 (comment)).
Six commits:
minmax(126px, 200px)counted tracks from the max, so 11 ports drew three ragged rows. The card
drops its up/down dot, cuts the name with an ellipsis plus a tooltip, and
keeps one layout whether the port is linked or not.
start after
dom.content()swaps a section; the theme measured the wrongthing and picked the wrong reference on three page shapes. Held by 216 runs
over three engines × two releases × two layouts × two widths × three
densities.
section's gap (
luci-app-irqbalancereturns a bare widget), a meter's valuestays off its label in a card, the title button row and per-section Delete
get air, a value row draws no separator when none follows — and
hiddenhides, which a class's
displaywas overriding.an h2 and no h1; it now also names the router (Feature request: luci-theme-footstrap: show hostname on login screen #8961). Realtime axis labels
were 1.16:1 against a light panel — recoloured through a child combinator,
so Channel Analysis keeps the per-BSSID colour that ties each label to its
curve.
group and marks it ready with an attribute nothing was watching (luci-theme-footstrap: add new theme #8903).
--fs-type-2xs) instead of aliteral stated twice in a page rule.
Checked on OpenWrt 25.12 (apk) and 24.10 (opkg), ImmortalWrt 25.12 and
24.10, and a hardware router. CSS changes are diffed against a baseline run of
the same sheet rather than eyeballed.