Last Comment Bug 663100 - [highlighter] Re-implement the highlighting mechanism in HTML + flex box model
: [highlighter] Re-implement the highlighting mechanism in HTML + flex box model
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 642471
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-09 06:38 PDT by Paul Rouget [:paul]
Modified: 2011-06-10 03:04 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (8.08 KB, patch)
2011-06-10 02:27 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-06-09 06:38:35 PDT
The current implementation (bug 642471) is in Canvas. Using HTML will allow us to use CSS transitions for animations, and will make the design work easier.
Comment 1 Paul Rouget [:paul] 2011-06-10 02:27:38 PDT
Created attachment 538467 [details] [diff] [review]
patch v1
Comment 2 Mihai Sucan [:msucan] 2011-06-10 02:51:20 PDT
Comment on attachment 538467 [details] [diff] [review]
patch v1

Review of attachment 538467 [details] [diff] [review]:
-----------------------------------------------------------------

Really good work. I have a few nit picks, but r+!

Your highlighter.css changes also need to be made in pinstripe and winstripe.

I am taking this patch into the main highlighter patch. It's surgical in its changes, very clean and straight-forward. Thank you!

(I applied all the nits my self, so I was able to test the patch. So no need for an update.)

::: browser/base/content/highlighter.xhtml
@@ +48,5 @@
> +    divs, organized in 3 rows, keeping the div in the middle transparent.
> +-->
> +<div id="veil-container">
> +<div id="veil">
> +    <div id="veil-topbox" class="veil"></div>

The other empty divs are marked as <div /> but this one is <div></div>.

@@ +56,5 @@
> +        <div id="veil-rightbox" class="veil"/>
> +    </div>
> +    <div id="veil-bottombox" class="veil"/>
> +</div>
> +</div>

Indentation is not consistent, and it should be two spaces.

::: browser/themes/gnomestripe/browser/highlighter.css
@@ +16,5 @@
>    z-index: 1;
>  }
>  
> +.veil {
> +    background-color: rgba(0, 0, 0, 0.5);

Indentation needs to follow the same level as in the rest of the file: two spaces.

@@ +21,3 @@
>  }
> +.veil, #veil-middlebox, #veil-transparentbox {
> +    -moz-transition: 0.2s;

I'd say 0.2s is too slow. I am going for 0.1s.

@@ +25,4 @@
>  }
> +#veil-container {
> +    position: absolute;
> +    top: 0; left: 0;

Each property needs to be on its own line.

Note You need to log in before you can comment on or make changes to this bug.