Skip to content

Commit

Permalink
Generalize the CSS rules for labeled toolbar buttons
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@timvandermeij @calixteman
timvandermeij and calixteman committed Aug 12, 2024
1 parent 68bda90 commit 9fd23d8
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 42 deletions.
44 changes: 23 additions & 21 deletions web/viewer.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1213,27 +1213,8 @@ dialog :link {
z-index: 50000; /* should be higher than anything else in PDF.js! */
}

#secondaryToolbar {
background-color: var(--doorhanger-bg-color);
cursor: default;
font: message-box;
font-size: 12px;
height: auto;
inset-inline-end: 4px;
line-height: 14px;
margin: 4px 2px;
padding: 6px 0 10px;
position: absolute;
text-align: left;
top: var(--toolbar-height);
z-index: 30000;

:is(button, a) {
font: message-box;
outline: none;
}

.toolbarButton {
.toolbarButton {
&.labeled {
border-radius: 0;
display: inline-block;
height: auto;
Expand Down Expand Up @@ -1273,6 +1254,27 @@ dialog :link {
padding-inline-end: 4px;
}
}
}

#secondaryToolbar {
background-color: var(--doorhanger-bg-color);
cursor: default;
font: message-box;
font-size: 12px;
height: auto;
inset-inline-end: 4px;
line-height: 14px;
margin: 4px 2px;
padding: 6px 0 10px;
position: absolute;
text-align: left;
top: var(--toolbar-height);
z-index: 30000;

:is(button, a) {
font: message-box;
outline: none;
}

#secondaryToolbarButtonContainer {
margin-bottom: -4px;
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="toolbarButton" type="button" title="Add image" tabindex="108" data-l10n-id="pdfjs-editor-stamp-add-image-button">
<button id="editorStampAddImage" class="toolbarButton labeled" 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="toolbarButton" type="button" title="Open File" tabindex="51" data-l10n-id="pdfjs-open-file-button">
<button id="secondaryOpenFile" class="toolbarButton labeled" 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="toolbarButton visibleMediumView" type="button" title="Print" tabindex="52" data-l10n-id="pdfjs-print-button">
<button id="secondaryPrint" class="toolbarButton labeled 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="toolbarButton visibleMediumView" type="button" title="Save" tabindex="53" data-l10n-id="pdfjs-save-button">
<button id="secondaryDownload" class="toolbarButton labeled 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="toolbarButton" type="button" title="Switch to Presentation Mode" tabindex="54" data-l10n-id="pdfjs-presentation-mode-button">
<button id="presentationMode" class="toolbarButton labeled" 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="toolbarButton" title="Current Page (View URL from Current Page)" tabindex="55" data-l10n-id="pdfjs-bookmark-button">
<a href="#" id="viewBookmark" class="toolbarButton labeled" 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="toolbarButton" type="button" title="Go to First Page" tabindex="56" data-l10n-id="pdfjs-first-page-button">
<button id="firstPage" class="toolbarButton labeled" 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="toolbarButton" type="button" title="Go to Last Page" tabindex="57" data-l10n-id="pdfjs-last-page-button">
<button id="lastPage" class="toolbarButton labeled" 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="toolbarButton" type="button" title="Rotate Clockwise" tabindex="58" data-l10n-id="pdfjs-page-rotate-cw-button">
<button id="pageRotateCw" class="toolbarButton labeled" 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="toolbarButton" type="button" title="Rotate Counterclockwise" tabindex="59" data-l10n-id="pdfjs-page-rotate-ccw-button">
<button id="pageRotateCcw" class="toolbarButton labeled" 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="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">
<button id="cursorSelectTool" class="toolbarButton labeled 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="toolbarButton" 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 labeled" 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="toolbarButton" 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 labeled" 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="toolbarButton 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 labeled 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="toolbarButton" 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 labeled" 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="toolbarButton" 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 labeled" 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="toolbarButton 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 labeled 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="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">
<button id="spreadOdd" class="toolbarButton labeled" 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="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">
<button id="spreadEven" class="toolbarButton labeled" 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="toolbarButton 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 labeled 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="toolbarButton" type="button" title="Document Properties…" tabindex="70" data-l10n-id="pdfjs-document-properties-button" aria-controls="documentPropertiesDialog">
<button id="documentProperties" class="toolbarButton labeled" 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 9fd23d8

Please sign in to comment.