luci-theme-footstrap: fixes from the field and from review - #8981
luci-theme-footstrap: fixes from the field and from review#8981VizzleTF wants to merge 5 commits into
Conversation
BKPepe
left a comment
BKPepe
left a comment
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.
643d8c9 to
1bf17fa
Compare
|
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
left a comment
openwrt-ai
left a comment
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
1bf17fa to
a241def
Compare
Do you have a ucode minification routine that could be imported here? Examples: |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the re-roll from twelve commits to six. All four inline comments from the previous round are addressed in the new series: the graph-label rule now uses a child combinator so Channel Analysis keeps its per-BSSID colours (confirmed against channel_analysis.js — the axis labels go in through createGraphText(G, …) where G is the <svg> root, while the per-BSSID labels sit inside a <g>), the sysauth.ut heading-level comment is corrected, --fs-type-2xs replaces the hardcoded size, and the fs-overview.js clamp comment now matches the CSS. The net content delta since 1bf17fa is small and looks right.
Two of the three commit/diff mismatches from the previous round are fixed by the re-roll — the meter-in-a-card and title-button/Delete hunks now sit in the spacing commit that describes them. What is left:
Commit checks
7260b51"luci-theme-footstrap: fix spacing and hiding on luci-base classes" — still carries.cbi-section .table + h3{margin-top:var(--fs-space-5)}and.cbi-section h3 + .table{margin-top:var(--fs-space-3)}. These moved from38c414einto this commit, but no message in the series describes them: the four items this one lists are the.cbi-map > *gap, the meter in a card, the title-button row plus per-section Delete, and the.cbi-valuehairline, and none of them is a heading adjacent to a table. Either add a fifth bullet or drop the two rules.a241def"luci-theme-footstrap: give the port card figures a size token" — the message is entirely about--fs-type-2xsandcascade.css, but the commit also rewrites two lines ofsysauth.ut(see inline). That commit should not exist at all once the token lands in0604294and the comment fix lands in458c87ad.- All six commits — the message bodies are hard-wrapped twice, leaving orphaned continuation lines:
0604294has "150px of", "from", "shares", "rows";326beb3has "reference", "broken", "the", "flick";458c87adhas "and", "one, at", "wordmark and", "for", "legible"; and so on in the other three. This predates the re-roll rather than being introduced by it, but since the series is being rewritten for @BKPepe's squash request anyway, a re-wrap at a single width would be worth doing in the same pass.
Generated by Claude Code
| abbr[title],acronym[title]{border-bottom:1px dotted;font-weight:inherit;cursor:help} | ||
| table{border-collapse:collapse;border-spacing:0} | ||
| ol,ul{list-style:none} | ||
| [hidden]:not([hidden="until-found"]){display:none !important} |
There was a problem hiding this comment.
This general rule retires more than the one point fix the commit message names. !important beats a non-important declaration regardless of cascade layer, so three further [hidden] restatements in this same sheet are now dead:
.fs-search-note[hidden]{display:none}at cascade.css:905.fs-ap-body[hidden]{display:none}at cascade.css:1042.cbi-value[hidden],.fs-ap-bgprev[hidden],.fs-ap-bgrow .btn[hidden]{display:none}at cascade.css:1069
The commit message says "the theme's own point fix for the same fault -- a restated display:none for the search overlay -- goes away with the general one", and only .fs-search-ov[hidden] was dropped. Dropping the other three too would keep the claim true and leave one place that decides what hidden means.
The same change also falsifies a rationale comment that is left in place: fs-appearance.js:383-385`` still says the restatement exists "at a specificity that beats .cbi-value's own display — the UA's bare `[hidden]` rule loses to it", which is no longer the situation once the theme ships its own `!important` rule.
Generated by Claude Code
| body[data-page="admin-status-overview"] .ifacebox:has(img[src*="/port_"]) > .ifacebox-body:nth-child(4){order:2;text-align:end;flex:1 0 auto} | ||
| body[data-page="admin-status-overview"] .ifacebox:has(img[src*="/port_"]) > .ifacebox-body:nth-child(4) > .cbi-tooltip-container{font-family:var(--fs-font-mono);font-size:var(--fs-type-xs) !important;color:var(--fs-dim);line-height:var(--fs-leading);white-space:nowrap;text-align:end !important} | ||
| body[data-page="admin-status-overview"] .ifacebox:has(img[src*="/port_"]) > .ifacebox-body:nth-child(4){order:2;text-align:end;flex:1 0 100%} | ||
| body[data-page="admin-status-overview"] .ifacebox:has(img[src*="/port_"]) > .ifacebox-body:nth-child(4) > .cbi-tooltip-container{font-family:var(--fs-font-mono);font-size:var(--fs-type-2xs) !important;color:var(--fs-dim);line-height:var(--fs-leading-tight);white-space:nowrap;text-align:end !important} |
There was a problem hiding this comment.
nit: introduce-then-fix inside the re-rolled series. 0604294 writes this rule with calc(10px * var(--fs-density-type)) stated twice (plainly plus an @supports override) and spends a paragraph of its message defending that shape — "Their size is declared plainly in the rule and rounded in an @supports block, the way 02-tokens.css states the type ladder". a241def, the last commit, then deletes both statements and the @supports block and replaces them with --fs-type-2xs.
Since the split is being re-rolled anyway (#8981 review by @BKPepe), landing --fs-type-2xs directly in 0604294 and dropping that paragraph would leave the series with no commit that ships a value it argues for and then removes. a241def would disappear entirely.
Generated by Claude Code
| A <p> and NOT a heading: the card's h1 is `Authorization Required` and stays the page's one | ||
| heading. An outline is about the page's SUBJECT, and a heading above that one would announce | ||
| the login form as two sections — a screen reader walking the headings would hear the router's | ||
| name as a section title with the form nested under it. It is printed above the h1 and centred |
There was a problem hiding this comment.
nit: this h2 -> h1 correction is carried by a241def, whose subject is "give the port card figures a size token" — the only other file that commit touches is cascade.css, and nothing in its message mentions sysauth.ut. The comment it corrects was introduced two commits earlier by 458c87ad, which is also the commit that promotes the heading to <h1> on line 115.
Fold it into 458c87ad so the wrong level is never committed, rather than shipping it and correcting it at the end of the series.
Generated by Claude Code
| * motion sampler starts and after it finishes, which in WebKit are 1.5 seconds apart. Waiting a | ||
| * flat interval instead made tools/scroll-anchor.mjs report a jump on every WebKit run and none | ||
| * on the other two engines, with the theme identical on all three. */ | ||
| restAt: () => _restAt, |
There was a problem hiding this comment.
This adds a public method to the shipped fs-fit baseclass that nothing in openwrt/luci calls — git grep restAt over the tree at this head matches only the private _restAt inside this file and this accessor. Its own doc comment names its consumer as tools/scroll-anchor.mjs, which is not part of this repository (themes/luci-theme-footstrap/ here contains only LICENSE, Makefile, htdocs, po, root, ucode).
Since the theme is minified on package build, a test-only accessor with no in-tree caller is dead weight that also reads as API to anyone extending the module. Either drop it from the exported object and let the harness reach _restAt another way, or say in the comment that the consumer lives out of tree so the next reader does not go looking for tools/.
Generated by Claude Code
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. Their size is a token of its own, --fs-type-2xs: one step below the ladder's smallest, rounded in the same @supports block that rounds the rest of it. The figures are the widest thing a card must fit whole, so 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>
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. Four restatements of the attribute go
with
it -- the search overlay's, the Appearance body's, the wallpaper
row/preview/button trio and the search note's -- all of them dead the
moment
the general rule landed, and all of them still reading as the reason those
elements hide.
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. The note beside the hostname is corrected in the same pass: it still called the card's heading an h2, which made its own argument read backwards. 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>
a241def to
4c0ecf1
Compare
4 participants
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)).
Five 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, keeps
one layout whether the port is linked or not, and sizes its figures with a
token of their own (
--fs-type-2xs).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, retiring four restatementsof the attribute with it.
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).
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.