Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the secondaryToolbarButton CSS class #18596

Merged

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Aug 11, 2024

Secondary toolbar buttons are toolbar buttons with some extra rules, mainly to make them wider and have visible labels. However, this similarity is currently not clearly reflected in the implementation because the secondary toolbar buttons use a different CSS class, secondaryToolbarButton, compared to the other toolbar buttons that use the toolbarButton CSS class. This also causes some duplication in the rules and requires extra care to keep the common bits for the secondaryToolbarButton class in sync with the toolbarButton class.

Fortunately, now that we have a dedicated CSS scope for the secondary toolbar, we can simplify this by giving all secondary toolbar buttons the toolbarButton class and explicitly listing the required overrides in the #secondaryToolbar scope instead. Doing so removes most of the special-casing for secondary toolbar buttons while explicitly listing the differences in a single place for a better overview. It also lays the foundation for making all toolbar buttons respect the browser.uidensity Firefox preference later by reducing differences.

Extracts a part of #18385.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/98b8fb83b3be0fd/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/98b8fb83b3be0fd/output.txt

Total script time: 1.02 mins

Published

web/viewer.css Outdated Show resolved Hide resolved
web/viewer.css Outdated Show resolved Hide resolved
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/3b92cec1105f329/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/3b92cec1105f329/output.txt

Total script time: 1.02 mins

Published

@Snuffleupagus
Copy link
Collaborator

In the master branch the .toggled class overrides the :hover effect, for the secondaryToolbar buttons, which this patch seem to "break"; I'm assuming this isn't intended?

Secondary toolbar buttons are toolbar buttons with some extra rules,
mainly to make them wider and have visible labels. However, this
similarity is currently not clearly reflected in the implementation
because the secondary toolbar buttons use a different CSS class,
`secondaryToolbarButton`, compared to the other toolbar buttons that
use the `toolbarButton` CSS class. This also causes some duplication
in the rules and requires extra care to keep the common bits for the
`secondaryToolbarButton` class in sync with the `toolbarButton` class.

Fortunately, now that we have a dedicated CSS scope for the secondary
toolbar, we can simplify this by giving all secondary toolbar buttons
the `toolbarButton` class and explicitly listing the required overrides
in the `#secondaryToolbar` scope instead. Doing so removes most of the
special-casing for secondary toolbar buttons while explicitly listing
the differences in a single place for a better overview. It also lays
the foundation for making all toolbar buttons respect the
`browser.uidensity` Firefox preference later by reducing differences.

Co-authored-by: Calixte Denizet <calixte.denizet@gmail.com>
@timvandermeij
Copy link
Contributor Author

Good catch! The original rule only overrode the secondary toolbar button style for the non-toggled case but I forgot to take that along in the new rule. This is fixed in the new commit and hovering over the toolbar buttons is now the same as before again.

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/3f84cec2ce362f4/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/3f84cec2ce362f4/output.txt

Total script time: 1.03 mins

Published

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you.

}
}

a.toolbarButton {
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Aug 11, 2024

Choose a reason for hiding this comment

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

Could this be moved into the .toolbarButton block above?

@timvandermeij timvandermeij merged commit aa2337f into mozilla:master Aug 11, 2024
6 checks passed
@timvandermeij timvandermeij deleted the css-secondary-toolbar-part-2 branch August 11, 2024 16:25
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Aug 12, 2024
This commit fixes a regression from mozilla#18596 where the "Add image" button
styles broke because it's a labeled toolbar button but the labeled
toolbar button CSS rules were only scoped to the secondary toolbar.
However, nowadays labeled toolbar buttons are also used in e.g. the
editor parameters toolbar, and this highlighted that we didn't clearly
encode the concept of labeled toolbar buttons in the CSS so far.

This patch extracts the CSS rules for labeled toolbar buttons that were
previously limited to the secondary toolbar into a dedicated generic
class that can be applied on top of any unlabeled toolbar button to
convert it to a labeled toolbar button, regardless of its position in
the DOM. This also makes the distinction clearer in the HTML, and the
new CSS scope for the toolbar buttons lays the foundation for combining
the other toolbar buttons rules in there later on.

Co-authored-by: Calixte Denizet <calixte.denizet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants