Skip to content

Commit

Permalink
Remove the secondaryToolbarButton CSS class
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@timvandermeij @calixteman
timvandermeij and calixteman committed Aug 11, 2024
1 parent efe00cf commit 2a22424
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 73 deletions.
104 changes: 52 additions & 52 deletions web/viewer.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,6 @@ body {
}

.toolbarButton,
.secondaryToolbarButton,
.dialogButton {
border: none;
background: none;
Expand All @@ -653,7 +652,7 @@ body {
overflow: hidden;
}

:is(.toolbarButton, .secondaryToolbarButton, .dialogButton)[disabled] {
:is(.toolbarButton, .dialogButton)[disabled] {
opacity: 0.5;
}

Expand All @@ -679,7 +678,6 @@ body {

.toolbarButton,
.dropdownToolbarButton,
.secondaryToolbarButton,
.dialogButton {
min-width: 16px;
margin: 2px 1px;
Expand All @@ -697,27 +695,23 @@ body {
.toolbarButton:is(:hover, :focus-visible) {
background-color: var(--button-hover-color);
}
.secondaryToolbarButton:is(:hover, :focus-visible) {
background-color: var(--doorhanger-hover-bg-color);
color: var(--doorhanger-hover-color);
}

:is(.toolbarButton, .secondaryToolbarButton).toggled,
.toolbarButton.toggled,
.splitToolbarButton.toggled > .toolbarButton.toggled {
background-color: var(--toggled-btn-bg-color);
color: var(--toggled-btn-color);
}

:is(.toolbarButton, .secondaryToolbarButton).toggled:hover,
.toolbarButton.toggled:hover,
.splitToolbarButton.toggled > .toolbarButton.toggled:hover {
outline: var(--toggled-hover-btn-outline) !important;
}

:is(.toolbarButton, .secondaryToolbarButton).toggled::before {
.toolbarButton.toggled::before {
background-color: var(--toggled-btn-color);
}

:is(.toolbarButton, .secondaryToolbarButton).toggled:hover:active,
.toolbarButton.toggled:hover:active,
.splitToolbarButton.toggled > .toolbarButton.toggled:hover:active {
background-color: var(--toggled-hover-active-btn-color);
}
Expand Down Expand Up @@ -765,7 +759,7 @@ body {
height: 1px;
}

:is(.toolbarButton, .secondaryToolbarButton, .treeItemToggler)::before,
:is(.toolbarButton, .treeItemToggler)::before,
.dropdownToolbarButton::after {
/* All matching images have a size of 16x16
* All relevant containers have a size of 28x28 */
Expand All @@ -789,17 +783,10 @@ body {
left: 6px;
}

.toolbarButton:is(:hover, :focus-visible)::before,
.secondaryToolbarButton:is(:hover, :focus-visible)::before {
.toolbarButton:is(:hover, :focus-visible)::before {
background-color: var(--toolbar-icon-hover-bg-color);
}

.secondaryToolbarButton::before {
opacity: var(--doorhanger-icon-opacity);
top: 5px;
inset-inline-start: 12px;
}

#sidebarToggle::before {
mask-image: var(--toolbarButton-sidebarToggle-icon);
transform: scaleX(var(--dir-factor));
Expand Down Expand Up @@ -850,15 +837,6 @@ body {
mask-image: var(--toolbarButton-download-icon);
}

a.secondaryToolbarButton {
padding-top: 5px;
text-decoration: none;
}
a:is(.toolbarButton, .secondaryToolbarButton)[href="#"] {
opacity: 0.5;
pointer-events: none;
}

#viewThumbnail::before {
mask-image: var(--toolbarButton-viewThumbnail-icon);
}
Expand Down Expand Up @@ -898,25 +876,6 @@ a:is(.toolbarButton, .secondaryToolbarButton)[href="#"] {
border-radius: 50%;
}

.secondaryToolbarButton {
position: relative;
margin: 0;
padding: 0 0 1px;
padding-inline-start: 36px;
height: auto;
min-height: 26px;
width: auto;
min-width: 100%;
text-align: start;
white-space: normal;
border-radius: 0;
box-sizing: border-box;
display: inline-block;
}
.secondaryToolbarButton > span {
padding-inline-end: 4px;
}

.verticalToolbarSeparator {
display: block;
margin: 5px 2px;
Expand Down Expand Up @@ -1264,6 +1223,47 @@ dialog :link {
}

#secondaryToolbar {
.toolbarButton {
border-radius: 0;
display: inline-block;
height: auto;
margin: 0;
padding: 0 0 1px;
padding-inline-start: 36px;
position: relative;
min-height: 26px;
min-width: 100%;
text-align: start;
white-space: normal;
width: auto;

&::before {
inset-inline-start: 12px;
opacity: var(--doorhanger-icon-opacity);
top: 5px;
}

&:not(.toggled):is(:hover, :focus-visible) {
background-color: var(--doorhanger-hover-bg-color);
color: var(--doorhanger-hover-color);
}

> span {
display: unset;
padding-inline-end: 4px;
}
}

a.toolbarButton {
padding-top: 5px;
text-decoration: none;

&[href="#"] {
opacity: 0.5;
pointer-events: none;
}
}

#secondaryToolbarButtonContainer {
margin-bottom: -4px;
max-height: calc(var(--viewer-container-height) - 40px);
Expand Down Expand Up @@ -1540,7 +1540,7 @@ dialog :link {
}

.visibleMediumView {
display: none;
display: none !important;
}

@media all and (max-width: 900px) {
Expand All @@ -1567,17 +1567,17 @@ dialog :link {
--editor-toolbar-base-offset: 40px;
}
#outerContainer .hiddenMediumView {
display: none;
display: none !important;
}
#outerContainer .visibleMediumView {
display: inherit;
display: inherit !important;
}
}

@media all and (max-width: 690px) {
.hiddenSmallView,
.hiddenSmallView * {
display: none;
display: none !important;
}
.toolbarButtonSpacer {
width: 0;
Expand Down
42 changes: 21 additions & 21 deletions web/viewer.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@

<div class="editorParamsToolbar hidden doorHangerRight" id="editorStampParamsToolbar">
<div class="editorParamsToolbarContainer">
<button id="editorStampAddImage" class="secondaryToolbarButton" type="button" title="Add image" tabindex="108" data-l10n-id="pdfjs-editor-stamp-add-image-button">
<button id="editorStampAddImage" class="toolbarButton" type="button" title="Add image" tabindex="108" data-l10n-id="pdfjs-editor-stamp-add-image-button">
<span class="editorParamsLabel" data-l10n-id="pdfjs-editor-stamp-add-image-button-label">Add image</span>
</button>
</div>
Expand All @@ -236,16 +236,16 @@
<div id="secondaryToolbar" class="secondaryToolbar hidden doorHangerRight">
<div id="secondaryToolbarButtonContainer">
<!--#if GENERIC-->
<button id="secondaryOpenFile" class="secondaryToolbarButton" type="button" title="Open File" tabindex="51" data-l10n-id="pdfjs-open-file-button">
<button id="secondaryOpenFile" class="toolbarButton" type="button" title="Open File" tabindex="51" data-l10n-id="pdfjs-open-file-button">
<span data-l10n-id="pdfjs-open-file-button-label">Open</span>
</button>
<!--#endif-->

<button id="secondaryPrint" class="secondaryToolbarButton visibleMediumView" type="button" title="Print" tabindex="52" data-l10n-id="pdfjs-print-button">
<button id="secondaryPrint" class="toolbarButton visibleMediumView" type="button" title="Print" tabindex="52" data-l10n-id="pdfjs-print-button">
<span data-l10n-id="pdfjs-print-button-label">Print</span>
</button>

<button id="secondaryDownload" class="secondaryToolbarButton visibleMediumView" type="button" title="Save" tabindex="53" data-l10n-id="pdfjs-save-button">
<button id="secondaryDownload" class="toolbarButton visibleMediumView" type="button" title="Save" tabindex="53" data-l10n-id="pdfjs-save-button">
<span data-l10n-id="pdfjs-save-button-label">Save</span>
</button>

Expand All @@ -255,82 +255,82 @@
<!-- <div class="horizontalToolbarSeparator visibleMediumView"></div>-->
<!--#endif-->

<button id="presentationMode" class="secondaryToolbarButton" type="button" title="Switch to Presentation Mode" tabindex="54" data-l10n-id="pdfjs-presentation-mode-button">
<button id="presentationMode" class="toolbarButton" type="button" title="Switch to Presentation Mode" tabindex="54" data-l10n-id="pdfjs-presentation-mode-button">
<span data-l10n-id="pdfjs-presentation-mode-button-label">Presentation Mode</span>
</button>

<a href="#" id="viewBookmark" class="secondaryToolbarButton" title="Current Page (View URL from Current Page)" tabindex="55" data-l10n-id="pdfjs-bookmark-button">
<a href="#" id="viewBookmark" class="toolbarButton" title="Current Page (View URL from Current Page)" tabindex="55" data-l10n-id="pdfjs-bookmark-button">
<span data-l10n-id="pdfjs-bookmark-button-label">Current Page</span>
</a>

<div id="viewBookmarkSeparator" class="horizontalToolbarSeparator"></div>

<button id="firstPage" class="secondaryToolbarButton" type="button" title="Go to First Page" tabindex="56" data-l10n-id="pdfjs-first-page-button">
<button id="firstPage" class="toolbarButton" type="button" title="Go to First Page" tabindex="56" data-l10n-id="pdfjs-first-page-button">
<span data-l10n-id="pdfjs-first-page-button-label">Go to First Page</span>
</button>
<button id="lastPage" class="secondaryToolbarButton" type="button" title="Go to Last Page" tabindex="57" data-l10n-id="pdfjs-last-page-button">
<button id="lastPage" class="toolbarButton" type="button" title="Go to Last Page" tabindex="57" data-l10n-id="pdfjs-last-page-button">
<span data-l10n-id="pdfjs-last-page-button-label">Go to Last Page</span>
</button>

<div class="horizontalToolbarSeparator"></div>

<button id="pageRotateCw" class="secondaryToolbarButton" type="button" title="Rotate Clockwise" tabindex="58" data-l10n-id="pdfjs-page-rotate-cw-button">
<button id="pageRotateCw" class="toolbarButton" type="button" title="Rotate Clockwise" tabindex="58" data-l10n-id="pdfjs-page-rotate-cw-button">
<span data-l10n-id="pdfjs-page-rotate-cw-button-label">Rotate Clockwise</span>
</button>
<button id="pageRotateCcw" class="secondaryToolbarButton" type="button" title="Rotate Counterclockwise" tabindex="59" data-l10n-id="pdfjs-page-rotate-ccw-button">
<button id="pageRotateCcw" class="toolbarButton" type="button" title="Rotate Counterclockwise" tabindex="59" data-l10n-id="pdfjs-page-rotate-ccw-button">
<span data-l10n-id="pdfjs-page-rotate-ccw-button-label">Rotate Counterclockwise</span>
</button>

<div class="horizontalToolbarSeparator"></div>

<div id="cursorToolButtons" role="radiogroup">
<button id="cursorSelectTool" class="secondaryToolbarButton toggled" type="button" title="Enable Text Selection Tool" tabindex="60" data-l10n-id="pdfjs-cursor-text-select-tool-button" role="radio" aria-checked="true">
<button id="cursorSelectTool" class="toolbarButton toggled" type="button" title="Enable Text Selection Tool" tabindex="60" data-l10n-id="pdfjs-cursor-text-select-tool-button" role="radio" aria-checked="true">
<span data-l10n-id="pdfjs-cursor-text-select-tool-button-label">Text Selection Tool</span>
</button>
<button id="cursorHandTool" class="secondaryToolbarButton" type="button" title="Enable Hand Tool" tabindex="61" data-l10n-id="pdfjs-cursor-hand-tool-button" role="radio" aria-checked="false">
<button id="cursorHandTool" class="toolbarButton" type="button" title="Enable Hand Tool" tabindex="61" data-l10n-id="pdfjs-cursor-hand-tool-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-cursor-hand-tool-button-label">Hand Tool</span>
</button>
</div>

<div class="horizontalToolbarSeparator"></div>

<div id="scrollModeButtons" role="radiogroup">
<button id="scrollPage" class="secondaryToolbarButton" type="button" title="Use Page Scrolling" tabindex="62" data-l10n-id="pdfjs-scroll-page-button" role="radio" aria-checked="false">
<button id="scrollPage" class="toolbarButton" type="button" title="Use Page Scrolling" tabindex="62" data-l10n-id="pdfjs-scroll-page-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-scroll-page-button-label">Page Scrolling</span>
</button>
<button id="scrollVertical" class="secondaryToolbarButton toggled" type="button" title="Use Vertical Scrolling" tabindex="63" data-l10n-id="pdfjs-scroll-vertical-button" role="radio" aria-checked="true">
<button id="scrollVertical" class="toolbarButton toggled" type="button" title="Use Vertical Scrolling" tabindex="63" data-l10n-id="pdfjs-scroll-vertical-button" role="radio" aria-checked="true">
<span data-l10n-id="pdfjs-scroll-vertical-button-label" >Vertical Scrolling</span>
</button>
<button id="scrollHorizontal" class="secondaryToolbarButton" type="button" title="Use Horizontal Scrolling" tabindex="64" data-l10n-id="pdfjs-scroll-horizontal-button" role="radio" aria-checked="false">
<button id="scrollHorizontal" class="toolbarButton" type="button" title="Use Horizontal Scrolling" tabindex="64" data-l10n-id="pdfjs-scroll-horizontal-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-scroll-horizontal-button-label">Horizontal Scrolling</span>
</button>
<button id="scrollWrapped" class="secondaryToolbarButton" type="button" title="Use Wrapped Scrolling" tabindex="65" data-l10n-id="pdfjs-scroll-wrapped-button" role="radio" aria-checked="false">
<button id="scrollWrapped" class="toolbarButton" type="button" title="Use Wrapped Scrolling" tabindex="65" data-l10n-id="pdfjs-scroll-wrapped-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-scroll-wrapped-button-label">Wrapped Scrolling</span>
</button>
</div>

<div class="horizontalToolbarSeparator"></div>

<div id="spreadModeButtons" role="radiogroup">
<button id="spreadNone" class="secondaryToolbarButton toggled" type="button" title="Do not join page spreads" tabindex="66" data-l10n-id="pdfjs-spread-none-button" role="radio" aria-checked="true">
<button id="spreadNone" class="toolbarButton toggled" type="button" title="Do not join page spreads" tabindex="66" data-l10n-id="pdfjs-spread-none-button" role="radio" aria-checked="true">
<span data-l10n-id="pdfjs-spread-none-button-label">No Spreads</span>
</button>
<button id="spreadOdd" class="secondaryToolbarButton" type="button" title="Join page spreads starting with odd-numbered pages" tabindex="67" data-l10n-id="pdfjs-spread-odd-button" role="radio" aria-checked="false">
<button id="spreadOdd" class="toolbarButton" type="button" title="Join page spreads starting with odd-numbered pages" tabindex="67" data-l10n-id="pdfjs-spread-odd-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-spread-odd-button-label">Odd Spreads</span>
</button>
<button id="spreadEven" class="secondaryToolbarButton" type="button" title="Join page spreads starting with even-numbered pages" tabindex="68" data-l10n-id="pdfjs-spread-even-button" role="radio" aria-checked="false">
<button id="spreadEven" class="toolbarButton" type="button" title="Join page spreads starting with even-numbered pages" tabindex="68" data-l10n-id="pdfjs-spread-even-button" role="radio" aria-checked="false">
<span data-l10n-id="pdfjs-spread-even-button-label">Even Spreads</span>
</button>
</div>

<div id="imageAltTextSettingsSeparator" class="horizontalToolbarSeparator hidden"></div>
<button id="imageAltTextSettings" type="button" class="secondaryToolbarButton hidden" title="Image alt text settings" tabindex="69" data-l10n-id="pdfjs-image-alt-text-settings-button" aria-controls="altTextSettingsDialog">
<button id="imageAltTextSettings" type="button" class="toolbarButton hidden" title="Image alt text settings" tabindex="69" data-l10n-id="pdfjs-image-alt-text-settings-button" aria-controls="altTextSettingsDialog">
<span data-l10n-id="pdfjs-image-alt-text-settings-button-label">Image alt text settings</span>
</button>

<div class="horizontalToolbarSeparator"></div>

<button id="documentProperties" class="secondaryToolbarButton" type="button" title="Document Properties…" tabindex="70" data-l10n-id="pdfjs-document-properties-button" aria-controls="documentPropertiesDialog">
<button id="documentProperties" class="toolbarButton" type="button" title="Document Properties…" tabindex="70" data-l10n-id="pdfjs-document-properties-button" aria-controls="documentPropertiesDialog">
<span data-l10n-id="pdfjs-document-properties-button-label">Document Properties…</span>
</button>
</div>
Expand Down

0 comments on commit 2a22424

Please sign in to comment.