• 
      

    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)