luci-theme-openwrt-2020: minor fixes - #8964
Conversation
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 5008956 "luci-theme-openwrt-2020: use lighter color for checkboxes" — the subject and body talk about checkboxes only, but the changed declaration sits in the shared
input[type="checkbox"], input[type="radio"]block andinput[type="radio"]:checked::afteralso paints withvar(--fg-color), so radio buttons are recoloured too. Either mention radios in the message or scope the override toinput[type="checkbox"].
Generated by Claude Code
| input[type="radio"] { | ||
| --bd-color: var(--main-dark-color); | ||
| --fg-color: var(--main-dark-color); | ||
| --fg-color: var(--main-bright-color); |
There was a problem hiding this comment.
This lowers the checked-state contrast in the theme's own (light) rendering. --fg-color paints the checkmark glyph via background: var(--fg-color) on :checked::after at line 1034, and the page background is white ([`body { background: var(--secondary-bright-color) }`](https://github.com/openwrt/luci/blob/500895691b08c46ab9da396a3ea3e8e4f1b0fbbd/themes/luci-theme-openwrt-2020/htdocs/luci-static/openwrt2020/cascade.css#L50-L51),`` #FFFFFF). #00B5E2 on white is roughly 2.4:1, under the 3:1 that WCAG 2.1 SC 1.4.11 asks for on non-text UI components; the current --main-dark-color (#002B49) is roughly 14.6:1.
This theme ships no prefers-color-scheme handling, so the only rendering it controls is the light one — and the commit message says the motivation is a third-party extension that inverts the page. Is trading the default rendering for that the intent? A @media (prefers-color-scheme: dark) override of --fg-color, or brightening only the ::before box outline and leaving the glyph dark, would fix the dark case without regressing the light one.
Generated by Claude Code
There was a problem hiding this comment.
maybe we should consider changing the color because of WCAG 2.1 SC 1.4.11, but thats for another day...
5008956 to
32970c5
Compare
|
I see that using bumping the "rows" value also fixes the problem while not blocking a scrollbar from appearing in other contexts. I think this is ready now |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push: the CSS overflow-y approach was replaced by a JS rows + 1 approach, so the first commit is entirely new. The previous commit-check on the checkbox commit is addressed — 32970c5 now documents the radio-button side effect.
Commit checks
- db70415 "treewide: fix textarea double scrolling issue" — the body says "add 1 to the dynamic size of the
textareaboxes", but in five of the seven files the value being incremented has already been padded by its producer, so the effective change is +2, not +1. The message also reads the pre-existing+ 1as belonging to unrelated files ("this+ 1already exists in some instances of similar Javascript files") when it is in fact the same padding, in the same files this commit touches. Either drop the duplicate hunks or state that +2 is intended. Details inline.
Generated by Claude Code
| const log = await this.retrieveLog(); | ||
| element.value = log?.value; | ||
| element.rows = log?.rows; | ||
| element.rows = log?.rows + 1; |
There was a problem hiding this comment.
retrieveLog() already returns the padded value — rows: loglines?.length + 1 at views.js:126. So log?.rows is already lines + 1, and this hunk makes the syslog box lines + 2. The commit message says "add 1 to the dynamic size", not 2, so this looks unintended — the + 1 the message notes "already exists in some instances of similar Javascript files" is this same producer-side one, not a separate concern.
| element.rows = log?.rows + 1; | |
| element.rows = log?.rows; |
The identical double-apply is in the four other log views, each of which already pads at the producer:
producer (already + 1) |
sites this PR adds a second + 1 |
|---|---|
| views.js:126 | this line, and line 280 |
| dmesg.js:109 | dmesg.js:122, dmesg.js:287 |
| antiblock/log.js:16`` | log.js:28, log.js:81 |
| antiblock/statistics.js:16`` | statistics.js:28, statistics.js:64 |
If lines + 2 really is what fixed the scrolling for you in testing, then the fix belongs in the one producer expression per file rather than in the two consumers, and the commit message should say +2.
Generated by Claude Code
There was a problem hiding this comment.
this is on purpose, to encourage this behavior no matter what example is used for a new application
In "textarea" class elements, like the syslog, depending on the browser, it is difficult to scroll if moused over or after clicking or highlighting text. The browser perceives that there is scrolling space/content within the inner element because when the height of the element is exactly the same size as text inside, the text padding or other invisible content exceeds the size of the text box and the scroll bar is kept in the window. This causes scrolling within the element with a range of just a few pixels, and the rest of the page remains static and refuses to scroll. To fix this add 1 to the dynamic size of the "textarea" boxes, and also the initial values before the dynamic size is calculated. This "+ 1" already exists in some instances of similar Javascript files. A previous attempt at fixing this used "overflow-y" styling but that causes undesirable behavior in some "textarea" use cases. In some cases "+ 1" is added to both dynamic and final sizes, this can equate to a total addition of 2 lines, which is still reasonable and proper padding, and this promotes the habit of padding the textarea elements treewide, no matter what example is used to modify or create the javascript for a new app. The aria2 application ternary check for a minimum textarea size seems to be written in reversed logic. It should likely be a minimum of 20 lines, and the exact number + 1 otherwise, so fix that while at it. Tested on Chrome 64-bit Windows 10. Ref: 755061b ("themes: remove overflow-y property...") Signed-off-by: Michael Pratt <mcpratt@pm.me>
When using browser extensions for automatic dark mode on pages, the background is made too dark to see whether or not the checkboxes are being toggled or their current state. Use the brighter blue color for the check in the checkbox which is suitable for either view as a light mode or dark mode, and also makes the color of the check match the progress bar color. This color change also affects "radio buttons" which likely have the same problem as checkboxes, although there are almost no "radio buttons" in LuCI at all. Signed-off-by: Michael Pratt <mcpratt@pm.me>
32970c5 to
60e8555
Compare
|
@hnyman I handled the Claude comments, would you mind taking a look? |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the two changes since 32970c5.
The ui.js fix is correct — this.options.rows ? this.options.rows + 1 : this.options.rows keeps the unset (null/undefined) case falling through to dom.attr()'s null check, so no rows attribute is emitted and the previous one-row / NaN regressions are gone.
The double-+ 1 question on the log views is answered by the updated commit body and the reply on that thread, so I'm not re-raising it. One remaining issue on the aria2 ternary, inline.
Both commit messages match their diffs; the checkbox commit now documents the radio-button side effect.
Generated by Claude Code
| 'readonly': true, | ||
| 'wrap': 'off', | ||
| 'rows': data.rows >= 20 ? 20 : data.rows + 1 | ||
| 'rows': data.rows >= 20 ? data.rows + 1 : 20 |
There was a problem hiding this comment.
Flipping the ternary makes every short file render a 20-row box. The commit message reads the original as "reversed logic", but the 20 in res.content.split('\n', 20) at line 20`` is what shows it was a maximum: the producer deliberately stops counting at 20 because the consumer never wanted more than 20 rows. Capping the count only makes sense against a cap.
Concretely, with res.rows now being min(lines, 20) + 1:
- empty or 1-line
aria2.conf—res.rowsis 2,2 >= 20is false, so the box is 20 rows tall for one line of content. Before this PR it was 2. - 19 lines —
res.rowsis 20, so the>= 20branch fires one line early and yields 21. - 20+ lines —
res.rowssaturates at 21, sodata.rows + 1is 22 no matter whether the file has 20 lines or 5000. So the "exact number + 1" branch never actually tracks the line count; the cap is still in force, just at 22 now. A genuine "minimum 20, exact count above that" would also have to drop the20limit from thesplit().
Neither effect is mentioned in the commit body, and neither relates to the double-scrolling bug this commit is about. If the intent was to keep the cap and just add the padding row, this restores that and stays consistent with the + 1 already applied at line 20:
| 'rows': data.rows >= 20 ? data.rows + 1 : 20 | |
| 'rows': Math.min(data.rows, 21) |
If a 20-row minimum really is wanted for the aria2 views, that is a separate UI change and worth its own commit rather than riding along in a treewide scrolling fix.
Generated by Claude Code
There was a problem hiding this comment.
probably an intermediate commit would be best to change the behavior
2 participants
Pull request details
Tested on
Web browser(s): Win 10 64-bit Chrome 151.0.7922.169