Fix styling of file attachment navigation and backgrounds.

Review Request #13911 — Created May 30, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

The Previous/Next file navigation buttons had a few style issues on dark
mode, some regressions, and bad interactions.

They were bunched up against the review UI, which regressed a while back
when we renamed the review-request CSS class to rb-c-review-request.
That had intended to ensure enough space between the button and the
review UI, but that had broken. That spacing has been restored.

The colors used on the navigation buttons have been updated to work in
both light and dark modes.

Thumbnail backgrounds now use a darker color by default, and a matching
text color. This gives us text-based file attachment thumbnails that
look appropriate on dark mode. For consistency with rendering, we use
the "diff equals" theme color. We may want to revisit that down the road
when we build a proper CSS component for thumbnails, instead picking the
theme color for text-based attachments only.

And finally, an interaction issue with navigation has been fixed. We
were wrapping the entire attachment in an <a href>, but since that's
an inherently inline element and we're putting complex block-level
elements inside of it, the browsers are forced to try to compensate in
some form. They seem to do that these days by wrapping most inner
elements with their own <a href>, creating a mess of styles. The end
result doesn't even make for a fully-clickable attachment.

The solution is the same as what we use for file attachment thumbnails
on a review request. We now have an <a href> overlay that sits on top
of the whole element and consumes all click events, giving us the
ability to navigate but also to respect browser link interactions. The
result is a wider clickable area for navigation. This has also been made
accessible.

Tested previous and next navigation in both light and dark mode, with
different types of file attachments.

Tested the main review request thumbnails for image-based, PDF-based,
and text-based file attachments.

Summary ID
Fix styling of file attachment navigation and backgrounds.
The Previous/Next file navigation buttons had a few style issues on dark mode, some regressions, and bad interactions. They were bunched up against the review UI, which regressed a while back when we renamed the `review-request` CSS class to `rb-c-review-request`. That had intended to ensure enough space between the button and the review UI, but that had broken. That spacing has been restored. The colors used on the navigation buttons have been updated to work in both light and dark modes. Thumbnail backgrounds now use a darker color by default, and a matching text color. This gives us text-based file attachment thumbnails that look appropriate on dark mode. For consistency with rendering, we use the "diff equals" theme color. We may want to revisit that down the road when we build a proper CSS component for thumbnails, instead picking the theme color for text-based attachments only. And finally, an interaction issue with navigation has been fixed. We were wrapping the entire attachment in an `<a href>`, but since that's an inherently inline element and we're putting complex block-level elements inside of it, the browsers are forced to try to compensate in some form. They seem to do that these days by wrapping most inner elements with their own `<a href>`, creating a mess of styles. The end result doesn't even make for a fully-clickable attachment. The solution is the same as what we use for file attachment thumbnails on a review request. We now have an `<a href>` overlay that sits on top of the whole element and consumes all click events, giving us the ability to navigate but also to respect browser link interactions. The result is a wider clickable area for navigation. This has also been made accessible.
2f80ff333b583534a5e906f317e87fa65ca16582

Description From Last Updated

Instead of getting rid of <a> and its browser features, can we give it the file-thumbnail-overlay class, and instead of …

maubinmaubin

When looking at the html in Chrome dev tools, I see that this appears as <div class="file-thumbnail"> <div class="file-thumbnail">... </div> …

maubinmaubin
david
  1. Ship It!
  2. 
      
maubin
  1. 
      
  2. reviewboard/templates/reviews/ui/base.html (Diff revision 1)
     
     
     
     
    Show all issues

    Instead of getting rid of <a> and its browser features, can we give it the file-thumbnail-overlay class, and instead of having all of the content wrapped in the <a>, make the content a sibling of it. So:

    <div class="review-ui-prev-attachment">
     <div class="file-container">
      <div class="file">
       <div class="file-thumbnail-container">
         <a href="..." class="file-thumbnail-overlay"></a>
         <div class="file-thumbnail">...</div>
       </div>
       <div class="file-caption-container">
       ...
    

    I notice this is what we do for file attachment thumbnails on the review request page. I do like to use Ctrl+Click when navigating, so I wouldn't like to see it go.

    1. This is a much better approach.

  3. Show all issues

    When looking at the html in Chrome dev tools, I see that this appears as

    <div class="file-thumbnail">
     <div class="file-thumbnail">...
     </div>
    <div>
    

    So looks like the file-thumbnail div is included in the {{prev_file_attachment.thumbnail}}. Can we remove the outer div here?

  4. 
      
chipx86
maubin
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (fc4082b)