Conversation
|
There are still some corners that are not covered. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
ebbc9703"treewide: replace "btn" class in label elements with input" — theFixes: 62382556d713trailer names a commit that is not in the tree at this point in the series;62382556d713is only re-landed by239e0fbcafterwards. Either order the revert-of-revert first or drop the trailer.
Generated by Claude Code
| .modal > pre,.modal > textarea{white-space:pre-wrap;overflow:auto;width:100%} | ||
| .modal:has(.cbi-map){max-width:min(1100px,94vw)} | ||
| #modal_overlay .modal label.btn{white-space:normal;overflow-wrap:anywhere;min-width:0} | ||
| #modal_overlay .modal{white-space:normal;overflow-wrap:anywhere;min-width:0} |
There was a problem hiding this comment.
Now that the selector is the modal itself, the id raises specificity above .modal{…min-width:270px} at cascade.css:146 and every modal loses its 270px floor. min-width:0 was only needed to let the old flex label shrink.
| #modal_overlay .modal{white-space:normal;overflow-wrap:anywhere;min-width:0} | |
| #modal_overlay .modal{white-space:normal;overflow-wrap:anywhere} |
Generated by Claude Code
There was a problem hiding this comment.
cc @VizzleTF for your attention, a proposal here
|
so annoying, why did they do this lol... |
239e0fb to
de9c47d
Compare
|
updated to a better method that agrees with HTML standard for the modal not handling the dropdown menu issues yet, but I have a plan for that... |
The vertical-align property set to "bottom" is conflicting with the default vertical-align property for checkbox inputs and is causing the checkbox to hang below the text instead of being aligned with it on desktop browsers. A different solution might be necessary for mobile browsers. If an element absolutely must be bottom aligned, it can be given the "bottom" class. For now, use "text-bottom" value instead, which also results in a centered appearance. Fixes: cbd0d7c ("luci-theme-openwrt: modal/progressbar rules, mobile improvements") Signed-off-by: Michael Pratt <mcpratt@pm.me>
de9c47d to
44e214d
Compare
|
rebased due to c8a49e1 now that Footstrap specifies then I'll add more commits |
44e214d to
4f6f114
Compare
|
ok fine... no more buttons or labels as the container, just using the p block now |
In order to be consistent with most themes, add a max-width rule for all elements within a modal for the remaining themes that do not define this. Because of box-sizing rules in the 2020 theme, this must not be set for the modal header in order for the background color to fill the full width. Signed-off-by: Michael Pratt <mcpratt@pm.me>
Interactive inputs like checkboxes and radio buttons are commonly nested within a label element to manage the text and spacing and to allow the trailing text to be part of a click target for the input. Because of that, label elements behave similarly to a generic button, however, the styling for the generic button class is too broad for inputs, as we have to be consistent with how checkboxes are presented between the different pages and modals, while at the same time, balancing the color of the check or radio button toggle with it's background, which is usually not colored by the button class, in both light modes and dark modes, whether programmed or third-party generated, for each theme. This improves the consistency in how checkboxes and similar inputs are handled by removing the "btn" class from the remaining use cases, but continue using similar style rules in order to keep the critical padding and effects. Use an existing but underutilized class "cbi-checkbox" instead. Also, remove all style instances of label.btn, which, as explained is slightly oxymoronic as the two should be treated separately, and there should no longer be any instances of a label with "btn" class. For the Material theme, this leaves two .modal blocks which are merged. In order to remain consistent with other checkboxes, add a blank label element in the container with a "for" attribute referencing an id for the input element. This is used for certain themes, like Footstrap, to replace the checkbox altogether with a slide-style switch that is animated and colored according to the input state. Because this is a new addition for the Footstrap theme, changes to the styling for that theme is a special exception with much more modification than the rest. Specifically, use "gap" instead of "margin-inline-end" and use "flex-shrink" in order to properly space the text from the switch and prevent line wrapping from shrinking the width of the background of the switch despite good spacing. Because HTML standard does not allow a label to be within a label, use the parent block container p in order to style the checkboxes with text. It is possible to use "div", but that container type is overused in other cases. This forces a few more style changes in order to keep spacing and effects the same. Most significantly, the extra input/label elements are now used in order to expand the click target by setting an absolute position equal to the parent container. For the Footstrap theme, the invisible checkbox is expanded, and for all others, the extra label element used to make a switch in the Footstrap theme is expanded. This is done in order to prevent dead click zones from the lack of CSS Flex style display by superimposing a label, since the parent container covering the whole area isn't one. Finally, turn the raw text within the container after the checkbox into a label in order to keep the accessibility features that are lost by changing the container. In some cases, this caused setting word breaking to become necessary in the label. Thanks to the blank label being overlayed due to absolute positioning, the text label does not need the "for" attribute or any class in order to maintain a full click target, which in turn allows for simpler styling blocks to distinguish between the two labels. For styling with all themes, use the directive "p.cbi-checkbox" so the styling for checkboxes with text afterward, especially the border or shadow, does not also affect checkboxes in a "div" element on other configuration pages. These elements should now be the only instances of a p block with "cbi-checkbox" class, and the only p blocks with an input and label pair using the "for" attribute. For all themes using inline-block display, add a right margin for inputs after clearing inherited margins to space the following text, if not already defined, as well as text-align set to "start" in the container to override browser defaults. Fixes: 6238255 ("luci-theme-openwrt-2020: use lighter color for checkboxes") Signed-off-by: Michael Pratt <mcpratt@pm.me>
4f6f114 to
7691af8
Compare
| body.push(E('p', { 'class': 'cbi-checkbox' }, [ | ||
| opts.keep[0], ' ', | ||
| E('label', { 'for': 'flash-keep' }), | ||
| E('label', {}, _('Keep settings and retain the current configuration')) |
There was a problem hiding this comment.
Dropping for leaves this label with neither an associated control nor a labelable descendant, so the only label bound to flash-keep is the empty one above and the checkbox ends up with an empty accessible name. Add aria-labelledby on the input pointing at this label (give it an id), or keep for here and distinguish the switch label some other way. Same at lines 263, 270 and 291 below, and at overview.js:239.``
Generated by Claude Code
| width: 100%; | ||
| } | ||
|
|
||
| p > label[for] { |
There was a problem hiding this comment.
This matches any <p> holding a label[for], not just the checkbox container, and such a <p> has no position: relative, so the label stretches over whatever positioned ancestor it finds and swallows clicks there. The "only p blocks with an input and label pair" invariant from the commit message holds for this tree but not for app views outside it, and scoping costs nothing. Same at material:2245, [openwrt2020:961](https://github.com/openwrt/luci/blob/7691af8943168ef5e572c924dc63378536128e78/themes/luci-theme-openwrt-2020/htdocs/luci-static/openwrt2020/cascade.css#L961``) and openwrt.org:636.``
| p > label[for] { | |
| p.cbi-checkbox > label[for] { |
Generated by Claude Code
| border: none; | ||
| margin: 0; |
There was a problem hiding this comment.
nit: bootstrap, material and openwrt.org scope the same margin to p > input / label > input (bootstrap:581-586),`` but here it lands on the bare element selector, so every checkbox and radio in the theme — table cells, dropdown lists, div.cbi-checkbox widgets — gains 0.5em trailing space and loses its UA margins. Move it into a `p.cbi-checkbox > input[type="checkbox"], p.cbi-checkbox > input[type="radio"]` rule to keep the change within the scope the commit describes.
Generated by Claude Code
|
I was hoping the AI would tell me there's actually more instances of a label within a label, or a label with cbi-checkbox class, or a cbi-checkbox class with an input/label pair, but I found at least one other on my own in |
|
@VizzleTF can you explain what's going on in this commit related to inputs? and what direction you're going? VizzleTF/luci-theme-footstrap@0bc6986 I was just experimenting on how to have both the switch and text within the same label element, which works by the way. some of what I tried can be implemented as a small improvement regardless which element is hosting the custom switch, however I feel like I wasted some time now... is there an excessive problem that made you now swap to directly replacing the checkbox instead of hiding it? or is it just because you saw what I was working on? I need to know so I can determine whether to keep working on this or wait for you first because now half of my commit message doesn't apply anymore. |
|
I mean I can read the reason
but why is an app hiding a label at all? |
Are you sure you ask the right person? Because they can idk xD PS I'm not planning further changes to how the switch is drawn. If anything, this should make your idea easier. The switch no longer depends on |
This branch has not been deployed
5 participants
ping @hnyman @DragonBluep
(
edit: quick summary of further development
adding a blank label inside elements used for checkbox inputs for consistency,
and ability to use the custom toggle in Footstrap theme in modals.
However, this causes label inside a label which is not HTML standard.
Any removal or replacement of the top level label causes click target voids.
Therefore we have to stretch either the label or input, whichever is invisible based on theme,
to the size of the parent container to cover the click target.
In cases where there is also text, this leads to having (for ARIA) two labels after every input.
also, style conflicts of color with both the checkbox buttons and dropdown selection lists
)
feel free to test yourselves, rebuild is not necessary, just copy
flash.jsand the respective
cascade.cssfor whatever theme you use...or try all of them, overwriting that file after installing,
just be sure that you overwrite the path in
/wwwand not/overlaywhich causes filesystem issues.hoping this makes everyone happy
can't wait for the AI to insult me somehow...
Pull request details
Fix the regression as described in #8964 and restore commit 6238255
Description
Fully rework how the checkboxes with directly following text are displayed for all themes (e.g. "Flash Image" modal)
bonus 2 commits of small quality adjustments
Screenshot or video of changes (if applicable)
a few for now, I can add more if desired
one of Footstrap which is a special case as described in commit message:
https://www.dropbox.com/scl/fi/69qljba8cvnot4b15y7zb/Screenshot-2026-09-17-07.09.14.png?rlkey=wgrriwcbh6pzm3k25xfvbcctz&dl=0
2020 theme light and dark (third-party app):
https://www.dropbox.com/scl/fi/vsbik2yustug6wciwhd54/Screenshot-2026-09-17-07.14.26.png?rlkey=09iw8uymakxdkqdnz69fl3wam&dl=0
https://www.dropbox.com/scl/fi/nwl3uy7iehn35s29o1j9o/Screenshot-2026-09-17-07.14.44.png?rlkey=a6ewrryjsajuvpm74zgndn8g1&dl=0
Tested on
OpenWrt version: OpenWrt SNAPSHOT (r0+35782-6f033b6cc3)
LuCI version: LuCI Master (26.225.36130~c62742f)
Web browser(s): Chrome 152.0.7977.83 (Official Build) (64-bit)