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

Bug 1511275 - Unify line numbers and source code listings in DOM. r=kats #221

Merged
merged 4 commits into from Jun 11, 2019

Conversation

asutherland
Copy link
Contributor

This also includes elements to fix bug 1557139.

This patch encompasses these changes (which can be split out):

  • Have the test 'searchfox' repo generate and use blame information. It's a
    bit difficult to iterate on blame and diff display without blame and the
    ability to thereby derive diff links. I've modified the test searchfox
    setup script to run blame generation for the self-same searchfox repo when
    indexer setup is run. The major downside to this is that if you have any
    local changes to the searchfox codebase that haven't been integrated into
    the blame data, then output-file may panic when the line counts aren't the
    same. This means an iteration loop looks like:

    • outside the vagrant VM
      • git commit -a --amend --reset-author
    • inside the vagrant VM
      • /vagrant/infrastructure/indexer-setup.sh /vagrant/tests config.json ~/index
      • rm -rf ~/index/tests/analysis
      • /vagrant/infrastructure/indexer-run.sh /vagrant/tests ~/index
      • /vagrant/infrastructure/web-server-run.sh /vagrant/tests ~/index ~
        I think there's probably a better status quo here that's possible. Maybe the
        test test repo could use git filter-branch so only changes to the test
        files generate the need for a blame rebuild.
  • Change the HTML structure as it relates to line number display.

    • Pre-patch, the source code display is a 2-column 1-row table where the left
      column contains the line numbers and the right column contains the source
      code lines. Additionally, the blame info is a child of the line number span
      which absolutely positions itself to the left of the line number.
    • Post-patch the blame, line number, and source line contents live together in
      a flexbox div. The blame and line-number have a fixed width whereas the
      source line grows to encompass the entire line.
    • The primary motivating factor is to make the position:sticky display both
      more understandable and more usable by having the line numbers (and blame)
      stick with the source line.
    • A secondary but still important concern is improving the accessibility of
      searchfox.
    • The line numbers live as generated content to avoid having them included in
      copied and pasted selections of code.
  • Fix bugs in the code-highlighter.js logic responsible for highlighting lines
    of code based on selection triggered by clicking on source code line numbers
    and optionally holding control and/or shift. This broke when the nesting
    divs were created for the position: sticky logic.

  • Implement a "scroll" event handler that marks the position:sticky stuck nodes
    as ".stuck" and ".last-stuck" so that:

    • Visual presentation can be improved. Previously position:sticky-able
      source lines were given a light background color so that they would
      hopefully distinguish themselves when stuck. This didn't work and added
      visual noise to normal source code display. By dynamically marking stuck
      lines of code with a class, we can choose a much more obvious styling that
      is only applied when the position:sticky lines become stuck. The downside
      is there may be a performance impact and the application of the style may
      not be immediate.
    • The behavior of clicking on a line number when the line is stuck can be
      changed from the existing selection logic to instead scroll up to the line
      number that was clicked on. For this reason, the stuck logic is (perhaps)
      confusingly include in code-highlighter.js.

@asutherland
Copy link
Contributor Author

I'm requesting review for a first-pass here. There are still a few things to be done here (at minimum):

  1. Make sure the a11y goals are met. :Jamie just gave me feedback in https://bugzilla.mozilla.org/show_bug.cgi?id=1511275#c7 so I'll apply that now.
  2. I think I may have regressed some padding a little bit. In order to get the "stuck" display to go edge-to-edge I did some refactoring that may not have been totally applied to the non source-listing views. This should be corrected.
  3. My "stuck" styling is almost certainly not perfect and could use some feedback from peoples.

This also includes elements to fix bug 1557139.

This patch encompasses these changes (which can be split out):

- Have the test 'searchfox' repo generate and use blame information.  It's a
  bit difficult to iterate on blame and diff display without blame and the
  ability to thereby derive diff links.  I've modified the test searchfox
  setup script to run blame generation for the self-same searchfox repo when
  indexer setup is run.  The major downside to this is that if you have any
  local changes to the searchfox codebase that haven't been integrated into
  the blame data, then output-file may panic when the line counts aren't the
  same.  This means an iteration loop looks like:
  - outside the vagrant VM
    - `git commit -a --amend --reset-author`
  - inside the vagrant VM
    - `/vagrant/infrastructure/indexer-setup.sh /vagrant/tests config.json ~/index`
    - `rm -rf ~/index/tests/analysis`
    - `/vagrant/infrastructure/indexer-run.sh /vagrant/tests ~/index`
    - `/vagrant/infrastructure/web-server-run.sh /vagrant/tests ~/index ~`
  I think there's probably a better status quo here that's possible.  Maybe the
  `test` test repo could use `git filter-branch` so only changes to the test
  files generate the need for a blame rebuild.

- Change the HTML structure as it relates to line number display.
  - Pre-patch, the source code display is a 2-column 1-row table where the left
    column contains the line numbers and the right column contains the source
    code lines.  Additionally, the blame info is a child of the line number span
    which absolutely positions itself to the left of the line number.
  - Post-patch the blame, line number, and source line contents live together in
    a flexbox div.  The blame and line-number have a fixed width whereas the
    source line grows to encompass the entire line.
  - The primary motivating factor is to make the position:sticky display both
    more understandable and more usable by having the line numbers (and blame)
    stick with the source line.
  - A secondary but still important concern is improving the accessibility of
    searchfox.
  - The line numbers live as generated content to avoid having them included in
    copied and pasted selections of code.

- Fix bugs in the code-highlighter.js logic responsible for highlighting lines
  of code based on selection triggered by clicking on source code line numbers
  and optionally holding control and/or shift.  This broke when the nesting
  divs were created for the position: sticky logic.

- Implement a "scroll" event handler that marks the position:sticky stuck nodes
  as ".stuck" and ".last-stuck" so that:
  - Visual presentation can be improved.  Previously position:sticky-able
    source lines were given a light background color so that they would
    hopefully distinguish themselves when stuck.  This didn't work and added
    visual noise to normal source code display.  By dynamically marking stuck
    lines of code with a class, we can choose a much more obvious styling that
    is only applied when the position:sticky lines become stuck.  The downside
    is there may be a performance impact and the application of the style may
    not be immediate.
  - The behavior of clicking on a line number when the line is stuck can be
    changed from the existing selection logic to instead scroll up to the line
    number that was clicked on.  For this reason, the stuck logic is (perhaps)
    confusingly include in code-highlighter.js.
Copy link
Contributor

@staktrace staktrace left a comment

Choose a reason for hiding this comment

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

Nice work! This looks and feels really good. I looked over all the code changes and they seem reasonable as well.

tools/src/format.rs Outdated Show resolved Hide resolved
static/css/mozsearch.css Show resolved Hide resolved
static/js/code-highlighter.js Outdated Show resolved Hide resolved
static/css/mozsearch.css Outdated Show resolved Hide resolved
- Restore default padding, add specialized class and selector to let the source
  listing use reduced padding for "full bleed" display to the edges of the body.
- Specialize highlighted line display to also include the faux bottom border.
- Specialize blame gutter to also include the faux bottom border.
- Refactor the faux bottom border stuff to use CSS variables so that it's less
  work to change the colors.
- Make the sticky logic not get confused by diff lines without a line number.
- Restore the blame pop-up to not live under the role=button.
@asutherland
Copy link
Contributor Author

Since the review changes were relatively small in scope and my local testing of the changes has gone well, I'm going to land this now with the expectation that there will probably be a follow-up on the styling mechanism. Also, there are definitely some further a11y improvements to be made as it relates to the blame popup.

@asutherland asutherland merged commit c0d10cd into mozsearch:master Jun 11, 2019
@asutherland asutherland deleted the move-lines branch June 11, 2019 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants