Bug 51162 – anchor the toolbar to the bottom of the diff if the diff doesn't take a full screen of height

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

anchor the toolbar to the bottom of the diff if the diff doesn't take a full screen of height

[Ojan Vafai]

Created attachment 76731 [details]
Patch

[Eric Seidel (no email)]

You're my hero!

[Adam Barth]

Comment on attachment 76731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76731&action=review

I'm glad we have some one working on the review tool who actually, you know, knows HTML+JavaScript+CSS.  :)

> BugsSite/code-review.js:637
> +    $(window).bind('resize', updateToolbarAnchorState);

Is this buggy if we're right on the boundary and someone adds an inline comment?  Or does that generate a resize event?

[Ojan Vafai]

Created attachment 76798 [details]
Patch

[Ojan Vafai]

Comment on attachment 76731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76731&action=review

>> BugsSite/code-review.js:637
>> +    $(window).bind('resize', updateToolbarAnchorState);
> 
> Is this buggy if we're right on the boundary and someone adds an inline comment?  Or does that generate a resize event?

Whoops. Good point. I've changed it to use a dummy iframe with percentage width/height and detect resizes on that iframe. That way, we'll trigger this code whenever the body's width/height changes. Since this is pretty different, I put it up for review again in case you have issue with it.

[Adam Barth]

Comment on attachment 76798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76798&action=review

Ok.  This patch is kind of crazy.  I'm slightly worried that this script is getting pretty complex.  Well, to be fair, it had kind of gotten out of control before this patch.  :)

> BugsSite/code-review.js:611
> +    if (hasScrollbar && !toolbar.hasClass('anchored'))
> +      toolbar.addClass('anchored')

Why not just add the class unconditionally?

> BugsSite/code-review.js:614
> +    if (!hasScrollbar)
> +      toolbar.removeClass('anchored');

I think there's a toggle concept in jQuery that helps with this.

> BugsSite/code-review.js:644
> +    $(document.body).append('<iframe id="pseudo_resize_event_iframe"></iframe>');
> +    document.getElementById('pseudo_resize_event_iframe').contentWindow.addEventListener('resize', onBodyResize);

I would have created the iframe elment as a fragment, using $('<iframe id="pseudo_resize_event_iframe"></iframe>'), appended the element, and then attached the event listener via the DOM directly instead of going back to the document to getElementById.  What if there's already an element with that ID in the document?  Anyway, it's not a big deal, it just has most assumptions.

This whole resize thing seems pretty crazy.  Is it really the best way?  I'm willing to believe you since you know way more about this stuff than I do.

[Ojan Vafai]

Comment on attachment 76798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76798&action=review

>> BugsSite/code-review.js:611
>> +      toolbar.addClass('anchored')
> 
> Why not just add the class unconditionally?

done.

>> BugsSite/code-review.js:614
>> +      toolbar.removeClass('anchored');
> 
> I think there's a toggle concept in jQuery that helps with this.

done.

>> BugsSite/code-review.js:644
>> +    document.getElementById('pseudo_resize_event_iframe').contentWindow.addEventListener('resize', onBodyResize);
> 
> I would have created the iframe elment as a fragment, using $('<iframe id="pseudo_resize_event_iframe"></iframe>'), appended the element, and then attached the event listener via the DOM directly instead of going back to the document to getElementById.  What if there's already an element with that ID in the document?  Anyway, it's not a big deal, it just has most assumptions.
> 
> This whole resize thing seems pretty crazy.  Is it really the best way?  I'm willing to believe you since you know way more about this stuff than I do.

Changed to create the iframe as a fragment. Changed the id to a classname since I need it for styling.

As for the resize thing, this is the best hack I know of. The proper fix would be to implement resize events on elements (https://bugs.webkit.org/show_bug.cgi?id=17969).

[Ojan Vafai]

Committed r75003: <http://trac.webkit.org/changeset/75003>

[James Robinson]

the iframe resize hack causes crazy-fast flashing in chrome if the page just barely needs a vertical scrollbar and just straight up crashes firefox 3.6 and 4.0 (I think they're dispatching the resize event synchronously) if the window is resized just so.  not good!

[Ojan Vafai]

Comment on attachment 76798 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76798&action=review

> BugsSite/ChangeLog:4
> +

asdf

> BugsSite/ChangeLog:10
> +
> +        * PrettyPatch/PrettyPatch.rb:
> +        * code-review.js:
> +

qwer

[Ojan Vafai]

Whoops. Sorry, I was testing. I thought I cancelled the submit correctly. Guess not. :(

[Ojan Vafai]

Comment on attachment 76798 [details]
Patch

> diff --git a/BugsSite/ChangeLog b/BugsSite/ChangeLog
> index 75e4d4933fbd9dd6fa58349dafde41529e33d45e..438fc4dee5da06db8dd69846dbdfa0ddb31be34d 100644
> --- a/BugsSite/ChangeLog
> +++ b/BugsSite/ChangeLog
> @@ -1,5 +1,15 @@
>  2010-12-15  Ojan Vafai  <[e-mail]>
>  
> +        Reviewed by NOBODY (OOPS!).
> +
> +        anchor the toolbar to the bottom of the diff if the diff doesn't take a full screen of height
> +        https://bugs.webkit.org/show_bug.cgi?id=51162
> +
> +        * PrettyPatch/PrettyPatch.rb:
> +        * code-review.js:
> +
> [phone]  Ojan Vafai  <[e-mail]>
> +
>          Reviewed by Adam Barth.
>  
>          size status bubble to it's contents on the code review page
> diff --git a/BugsSite/PrettyPatch/PrettyPatch.rb b/BugsSite/PrettyPatch/PrettyPatch.rb
> index 1dd16811c67d344d894898095752c0a111087cd6..45b3a1a5e27275928b8d2ecb824856426149e0f7 100644
> --- a/BugsSite/PrettyPatch/PrettyPatch.rb
> +++ b/BugsSite/PrettyPatch/PrettyPatch.rb
> @@ -120,7 +120,7 @@ private
>      background-color: #f8f8f8;
>      border: 1px solid #ddd;
>      font-family: monospace;
> -    margin: 2em 0px;
> +    margin: 1em 0;
>  }
>  
>  h1 {
> @@ -236,16 +236,19 @@ body {
>  #toolbar {
>    display: -webkit-box;
>    display: -moz-box;
> -  position: fixed;
>    padding: 3px;
>    bottom: 0;
>    left: 0;
>    right: 0;
> -  border-top: 1px solid #ddd;
> +  border: 1px solid #ddd;
>    background-color: #eee;
>    font-family: sans-serif;
>  }
>  
> +#toolbar.anchored {
> +  position: fixed;
> +}
> +
>  #toolbar .actions {
>    float: right;
>  }
> @@ -349,10 +352,18 @@ body {
>  }
>  
>  .statusBubble {
> +  height: 20px;
>    margin: 2px 2px 0 0;
>    border: none;
>    vertical-align: middle;
>  }
> +
> +#pseudo_resize_event_iframe {
> +  height: 10%;
> +  width: 10%;
> +  position: absolute;
> +  top: -11%;
> +}
>  </style>
>  <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
>  <script src="code-review.js?version=16"></script>
> diff --git a/BugsSite/code-review.js b/BugsSite/code-review.js
> index c8872208d7da116dd743bf6c7a113f4f0d64d8be..b3e1caf0ee1c00d9b7e83040b74aa7ae1b1e3756 100644
> --- a/BugsSite/code-review.js
> +++ b/BugsSite/code-review.js
> @@ -600,11 +600,25 @@
>      $('#statusBubbleContainer').addClass('wrap');
>    }
>  
> +  function onBodyResize() {
> +    updateToolbarAnchorState();
> +  }
> +
> +  function updateToolbarAnchorState() {
> +    var hasScrollbar = window.innerWidth > document.documentElement.offsetWidth;
> +    var toolbar = $('#toolbar');
> +    if (hasScrollbar && !toolbar.hasClass('anchored'))
> +      toolbar.addClass('anchored')
> +
> +    if (!hasScrollbar)
> +      toolbar.removeClass('anchored');
> +  }
> +
>    $(document).ready(function() {
>      crawlDiff();
>      fetchHistory();
>      $(document.body).prepend('<div id="message"><div class="help">Select line numbers to add a comment.</div><div class="commentStatus"></div></div>');
> -    $(document.body).prepend('<div id="toolbar">' +
> +    $(document.body).append('<div id="toolbar">' +
>          '<div class="overallComments">' +
>              '<textarea placeholder="Overall comments"></textarea>' +
>          '</div>' +
> @@ -622,6 +636,14 @@
>      $('.overallComments textarea').bind('click', openOverallComments);
>  
>      $(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe id="reviewform" src="attachment.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
> +
> +    // Create a dummy iframe and monitor resizes in it's contentWindow to detect when the top document's body changes size.
> +    // Using jQuery to get the Window or add the event listener throws an error here.
> +    // Not sure why, but it didn't seem worth stepping into jQuery obfuscated code to figure out.
> +    $(document.body).append('<iframe id="pseudo_resize_event_iframe"></iframe>');
> +    document.getElementById('pseudo_resize_event_iframe').contentWindow.addEventListener('resize', onBodyResize);
> +
> +    updateToolbarAnchorState();
>    });
>  
>    function discardComment() {

[Ojan Vafai]

Comment on attachment 76798 [details]
Patch

> diff --git a/BugsSite/ChangeLog b/BugsSite/ChangeLog
> index 75e4d4933fbd9dd6fa58349dafde41529e33d45e..438fc4dee5da06db8dd69846dbdfa0ddb31be34d 100644
> --- a/BugsSite/ChangeLog
> +++ b/BugsSite/ChangeLog
> @@ -1,5 +1,15 @@
>  2010-12-15  Ojan Vafai  <[e-mail]>
>  
> +        Reviewed by NOBODY (OOPS!).
> +
> +        anchor the toolbar to the bottom of the diff if the diff doesn't take a full screen of height
> +        https://bugs.webkit.org/show_bug.cgi?id=51162
> +
> +        * PrettyPatch/PrettyPatch.rb:
> +        * code-review.js:
> +
> [phone]  Ojan Vafai  <[e-mail]>
> +
>          Reviewed by Adam Barth.
>  
>          size status bubble to it's contents on the code review page
> diff --git a/BugsSite/PrettyPatch/PrettyPatch.rb b/BugsSite/PrettyPatch/PrettyPatch.rb
> index 1dd16811c67d344d894898095752c0a111087cd6..45b3a1a5e27275928b8d2ecb824856426149e0f7 100644
> --- a/BugsSite/PrettyPatch/PrettyPatch.rb
> +++ b/BugsSite/PrettyPatch/PrettyPatch.rb
> @@ -120,7 +120,7 @@ private
>      background-color: #f8f8f8;
>      border: 1px solid #ddd;
>      font-family: monospace;
> -    margin: 2em 0px;
> +    margin: 1em 0;
>  }
>  
>  h1 {
> @@ -236,16 +236,19 @@ body {
>  #toolbar {
>    display: -webkit-box;
>    display: -moz-box;
> -  position: fixed;
>    padding: 3px;
>    bottom: 0;
>    left: 0;
>    right: 0;
> -  border-top: 1px solid #ddd;
> +  border: 1px solid #ddd;
>    background-color: #eee;
>    font-family: sans-serif;
>  }
>  
> +#toolbar.anchored {
> +  position: fixed;
> +}
> +
>  #toolbar .actions {
>    float: right;
>  }
> @@ -349,10 +352,18 @@ body {
>  }
>  
>  .statusBubble {
> +  height: 20px;
>    margin: 2px 2px 0 0;
>    border: none;
>    vertical-align: middle;
>  }
> +
> +#pseudo_resize_event_iframe {
> +  height: 10%;
> +  width: 10%;
> +  position: absolute;
> +  top: -11%;
> +}
>  </style>
>  <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
>  <script src="code-review.js?version=16"></script>
> diff --git a/BugsSite/code-review.js b/BugsSite/code-review.js
> index c8872208d7da116dd743bf6c7a113f4f0d64d8be..b3e1caf0ee1c00d9b7e83040b74aa7ae1b1e3756 100644
> --- a/BugsSite/code-review.js
> +++ b/BugsSite/code-review.js
> @@ -600,11 +600,25 @@
>      $('#statusBubbleContainer').addClass('wrap');
>    }
>  
> +  function onBodyResize() {
> +    updateToolbarAnchorState();
> +  }
> +
> +  function updateToolbarAnchorState() {
> +    var hasScrollbar = window.innerWidth > document.documentElement.offsetWidth;
> +    var toolbar = $('#toolbar');
> +    if (hasScrollbar && !toolbar.hasClass('anchored'))
> +      toolbar.addClass('anchored')
> +
> +    if (!hasScrollbar)
> +      toolbar.removeClass('anchored');
> +  }
> +
>    $(document).ready(function() {
>      crawlDiff();
>      fetchHistory();
>      $(document.body).prepend('<div id="message"><div class="help">Select line numbers to add a comment.</div><div class="commentStatus"></div></div>');
> -    $(document.body).prepend('<div id="toolbar">' +
> +    $(document.body).append('<div id="toolbar">' +
>          '<div class="overallComments">' +
>              '<textarea placeholder="Overall comments"></textarea>' +
>          '</div>' +
> @@ -622,6 +636,14 @@
>      $('.overallComments textarea').bind('click', openOverallComments);
>  
>      $(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe id="reviewform" src="attachment.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
> +
> +    // Create a dummy iframe and monitor resizes in it's contentWindow to detect when the top document's body changes size.
> +    // Using jQuery to get the Window or add the event listener throws an error here.
> +    // Not sure why, but it didn't seem worth stepping into jQuery obfuscated code to figure out.
> +    $(document.body).append('<iframe id="pseudo_resize_event_iframe"></iframe>');
> +    document.getElementById('pseudo_resize_event_iframe').contentWindow.addEventListener('resize', onBodyResize);
> +
> +    updateToolbarAnchorState();
>    });
>  
>    function discardComment() {

[Ojan Vafai]

Sorry for all the noise. I'm done now.