Last Comment Bug 671291 - </iframe> and </html> overlap in the Inspect window
: </iframe> and </html> overlap in the Inspect window
Status: RESOLVED FIXED
[fixed-in-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 9
Assigned To: Kyle Simpson
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 688439
  Show dependency treegraph
 
Reported: 2011-07-13 08:59 PDT by :Ehsan Akhgari
Modified: 2011-09-23 15:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot (59.90 KB, image/png)
2011-07-13 08:59 PDT, :Ehsan Akhgari
no flags Details
fix and test for the bug (7.01 KB, patch)
2011-08-31 10:51 PDT, Kyle Simpson
rcampbell: review-
Details | Diff | Splinter Review
updated from feedback, removed tests (2.54 KB, patch)
2011-09-05 14:41 PDT, Kyle Simpson
rcampbell: review+
dao+bmo: review+
Details | Diff | Splinter Review
[in-fx-team] updated CSS comments per Dao (2.79 KB, patch)
2011-09-22 07:09 PDT, Kyle Simpson
getify: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-07-13 08:59:07 PDT
Created attachment 545670 [details]
Screenshot

See the screenshot to see the problem.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-07-18 08:50:42 PDT
I've seen this before too. Does this happen for you on a specific site?
Comment 2 Jan Honza Odvarko [:Honza] 2011-08-30 08:50:04 PDT
My first guess would be that the problem is not related to Domplate (the engine itself), but rather to templates (based on Domplate) that are used to generate the Inspect window UI. It could be also related to CSS.

How can I inspect content of the Inspect window. When using DOM Inspector, I can't find the window in: File -> Inspect Chrome Document list (it's not a browser window).

Honza
Comment 3 Kyle Simpson 2011-08-30 09:01:42 PDT
it is indeed a CSS issue, which i've fixed. but i'm trying now to figure out the proper way to write a test for it.
Comment 4 Kyle Simpson 2011-08-31 10:51:38 PDT
Created attachment 557238 [details] [diff] [review]
fix and test for the bug

CSS bug fix and test.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-09-01 17:27:49 PDT
Comment on attachment 557238 [details] [diff] [review]
fix and test for the bug

in:

+++ b/browser/themes/gnomestripe/browser/inspector.css
@@ -197,26 +197,26 @@ code {

line 139:
 .docType {
   position: absolute;
-  top: -16px;
+  top: -16px; /* position the DOCTYPE element above/outside the "nodeBox" that contains it */
   font-family: Menlo, Andale Mono, monospace;
   padding-left: 8px;

The comment line's a little too long. Maybe move that to the top of .docType and explain it there.

Same for .htmlNodeBox

and in all files.

Not sure we really need the unittest for this, though I guess it could save us if inadvertently regress due to a "simple style change" (lol).

in:

+++ b/browser/base/content/test/inspector/browser_inspector_bug_671291_doctype_node.

+ * 
+ * Contributor(s):
+ *   Kyle Simpson <ksimpson@mozilla.com> 

We don't usually include a Contributors block in PD-licensed code. Also, watch your trailing spaces! 

line 52:
+  Services.obs.removeObserver(runTreePanelTests, 

trailing space!

line 59:
+  // grab the test iframe's htmlNodeBox
+  let ifrHTMLNodeBox = 

trailing space. I'll stop doing that now. :)

line 60:
+  let ifrHTMLNodeBox = 
+        InspectorUI.treeBrowserDocument.querySelectorAll(".htmlNodeBox")[1];

use querySelector() instead of the qSA()[1].

line 68:
+  for (var i = 0; i

why not "let"?

+
+  if (htmlNodeBoxStyle.marginTop) {
+    allowedMarginOverlap += parseInt(htmlNodeBoxStyle.marginTop, 10);

I don't think you need to specify the , 10 in your parseInt();

lines 88, 91, 95, 89:
+  if (htmlNodeBoxStyle.marginTop) {

are these ever zero?

line 91:

+  else if (htmlNodeBoxStyle.marginBottom) {

why is this an "else" thing?

line 103:
+  ok((ifrHTMLNodeBoxDims.bottom - allowedMarginOverlap) <= closeLabelBoxDims.top, 

Do we even need to calculate an allowed margin overlap? If the nodes' bottom - top are negative (or maybe negative N), they're overlapping, aren't they?

r-. I don't think we really need this test.

r+ on the css changes with the comments moved, but we'll need a browser peer for final approval.

thanks!
Comment 6 Kyle Simpson 2011-09-05 14:41:34 PDT
Created attachment 558342 [details] [diff] [review]
updated from feedback, removed tests
Comment 7 Rob Campbell [:rc] (:robcee) 2011-09-06 14:57:22 PDT
Comment on attachment 558342 [details] [diff] [review]
updated from feedback, removed tests

ok with me.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-09-07 07:01:50 PDT
Comment on attachment 558342 [details] [diff] [review]
updated from feedback, removed tests

requesting some theme review.
Comment 9 Dão Gottwald [:dao] 2011-09-07 07:16:11 PDT
This code seems somewhat broken... you specify font-sizes in inspector.css but no line-height; it's not guaranteed that the 16px you're using in this patch actually matches the used font (in case Menlo and Andale Mono aren't available).
Comment 10 Rob Campbell [:rc] (:robcee) 2011-09-07 07:43:10 PDT
mm. Do we have any @monospace-line-height; CSS variables or similar that we could use for this?
Comment 11 Dão Gottwald [:dao] 2011-09-07 08:19:48 PDT
(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> mm. Do we have any @monospace-line-height; CSS variables or similar that we
> could use for this?

Nope. The "correct" way would be to specify a line-height for the whole tree or (preferably) fix the structure so that .docType isn't inside of .htmlNodeBox.
Comment 12 Kyle Simpson 2011-09-07 11:01:31 PDT
I think the markup and CSS for this particular part of the tree is not good. But, that code was checked in and reviewed prior to me being part of the team, so I had to go on the assumption that it was acceptable, and not try to reinvent anything. My goal was to fix a minor rendering bug in the least-intrusive way possible, by adjusting which CSS property was used to do the offsetting.

At first, I felt like the more correct, but certainly more involved, approach was to tackle re-organizing how ioBox handles things so that Doctype is not a child of the html element (as Dao has suggested), and discussed this idea with several team members. The prevailing opinion was that as a simple bug fix, the simpler CSS fix path was the right one, so that's how we went. It was a judgement call, but I now think it was the right one.

Also, in an earlier patch, I had a test designed specifically to fail if the assumptions about height and line-height were ever made out of sync. It was felt by the team that my test was unnecessary, so it was removed. Perhaps the test should be put back in, if this code is more a concern than at first thought?

I noted in the CSS comments what those relative 16px and -16px values were doing, and I felt like those comments (plus the tests if we add them) were sufficient to protect this part of the code from too much future breakage.
Comment 13 Dão Gottwald [:dao] 2011-09-07 11:10:03 PDT
(In reply to Kyle Simpson from comment #12)
> I think the markup and CSS for this particular part of the tree is not good.
> But, that code was checked in and reviewed prior to me being part of the
> team, so I had to go on the assumption that it was acceptable, and not try
> to reinvent anything.

Sorry, this happens.

> Also, in an earlier patch, I had a test designed specifically to fail if the
> assumptions about height and line-height were ever made out of sync. It was
> felt by the team that my test was unnecessary, so it was removed. Perhaps
> the test should be put back in, if this code is more a concern than at first
> thought?

It's an ineffective test, unless you're going to run it with a broad variety of monospace fonts.

By the way, the line heights are already out of sync in the screenshot. The vertical gap between <!DOCTYPE html> and <html> is larger than between the other lines.
Comment 14 Kyle Simpson 2011-09-07 11:30:26 PDT
(In reply to Dão Gottwald [:dao] from comment #13)
> > Also, in an earlier patch, I had a test designed specifically to fail if the
> > assumptions about height and line-height were ever made out of sync. It was
> > felt by the team that my test was unnecessary, so it was removed. Perhaps
> > the test should be put back in, if this code is more a concern than at first
> > thought?
> 
> It's an ineffective test, unless you're going to run it with a broad variety
> of monospace fonts.

If our testing platforms are not testing the variations of fonts available, and how those variations can affect firefox as a whole, then our testing setup is lacking, and that's a much bigger issue than this small ticket. It's important, of course, but not something that I think should hold up a small ticket bug fix.

My test was designed to make sure that there wasn't any overlap of boxes (except that which was specifically intended by the negative margin being set). I believe in the case you're referring to, if a font were used which didn't render at 16px, but instead rendered at 17px, and that font variation was something that our test systems were testing for, then we'd have a specific test fail. Such a test failure would clearly show that the current CSS isn't sufficient.

But we don't, to my knowledge, have such a scenario specifically defined where I can run a test and see a failure. In the tests I was able to run, manually and automated, it seemed to work just fine.

So at the very least, I think what's missing here (perhaps just in my own knowledge of firefox testing) is any definitive way to construct a scenario like you describe, where I can create a test that fails, and know that a solution is definitively fixing it.


> By the way, the line heights are already out of sync in the screenshot. The
> vertical gap between <!DOCTYPE html> and <html> is larger than between the
> other lines.

I think you may very well be correct that there's several CSS tweak issues that could eventually (and probably should) be addressed with this tree. There are actually a bunch of things I'd like to fix about it. 

In fact, I still have another ticket which is basically a nearly complete rewrite of the tree, and if that ticket is ever given priority, it would make most of what you're talking about here somewhat moot, because in that rewrite patch I already did, my code didn't suffer from these types of CSS assumptions.

I kinda think that a small bug ticket like this is much more limited in scope then what you're uncovering. What you're pointing out there about spacing between <DOCTYPE> and <html> is a separate problem that exists both before and after this patch, isn't made any worse (or better) by this patch, and is thus (IMHO) orthagonal.

I suggest a separate ticket to address the greater CSS issues that you have identified.
Comment 15 Kyle Simpson 2011-09-07 11:39:01 PDT
It seems likely that most variations of monospace font sizings would be 1 or 2px +/- of the 16px, and there's already whitespace padding built in between each line. So it seems unlikely (I think) that a monospace font issue as Dao describes would actually create a badly broken rendering -- at worst, lines might be closer together or further apart than in other font scenarios. I think it'd have to be 4 or more pixels +/- of the 16px measurement before we'd likely see unusable line overlapping.
Comment 16 Dão Gottwald [:dao] 2011-09-07 11:48:25 PDT
(In reply to Kyle Simpson from comment #14)
> If our testing platforms are not testing the variations of fonts available,
> and how those variations can affect firefox as a whole, then our testing
> setup is lacking, and that's a much bigger issue than this small ticket.

This isn't the fault of our testing setup. Code shouldn't make assumptions about font metrics, and then you don't need to test it.

> So at the very least, I think what's missing here (perhaps just in my own
> knowledge of firefox testing) is any definitive way to construct a scenario
> like you describe, where I can create a test that fails, and know that a
> solution is definitively fixing it.

We know that a solution that doesn't care about font metrics would fix any hypothetical failure here.

> I think you may very well be correct that there's several CSS tweak issues
> that could eventually (and probably should) be addressed with this tree.
> There are actually a bunch of things I'd like to fix about it. 
> 
> In fact, I still have another ticket which is basically a nearly complete
> rewrite of the tree, and if that ticket is ever given priority, it would
> make most of what you're talking about here somewhat moot, because in that
> rewrite patch I already did, my code didn't suffer from these types of CSS
> assumptions.

Great, what's the bug number?

> I kinda think that a small bug ticket like this is much more limited in
> scope then what you're uncovering. What you're pointing out there about
> spacing between <DOCTYPE> and <html> is a separate problem that exists both
> before and after this patch, isn't made any worse (or better) by this patch,
> and is thus (IMHO) orthagonal.

The problem is that if we always favor the quick fixes, the good fixes aren't going to happen. But if a correct and robust fix is in the works, I have no problem r+'ing this interim solution.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-09-21 05:28:41 PDT
dao? ping for review again or was that implicit in your previous comment?
Comment 18 Dão Gottwald [:dao] 2011-09-21 05:32:24 PDT
No, it wasn't. I asked a question, still waiting for a response.
Comment 19 Kyle Simpson 2011-09-21 05:42:05 PDT
The question Dao raises (shouldn't we re-organize ioBox instead of making CSS assumptions about pixels) is valid, but was already considered and was felt to be more trouble than it was worth for this bug. There are actually several things which should be addressed related to this topic, but that's outside the scope of the simple bug here, and so I recommended that it be a follow-up ticket to address the other things wrong with the html panel.

This bug-fix doesn't introduce any additional assumptions or brittleness that didn't already exist, it simply uses the existing CSS arrangement.
Comment 20 Dão Gottwald [:dao] 2011-09-21 05:50:41 PDT
Your patch doesn't make this code worse, but it shouldn't be needed in the first place. You said you had "another ticket which is basically a nearly complete rewrite of the tree". Where is it?
Comment 21 Kyle Simpson 2011-09-21 14:24:34 PDT
The bug I was referring to is bug #659710. It was indefinitely shelved a couple of months ago, based on team priorities. The code is not in sufficient state to be quickly resurrected and checked in. It needs a non-trivial amount of work to revive it. I do hope that it eventually is prioritized as such, but I have no idea when/if that will happen.

In any case, I don't rely on the same assumptions about font-size pixels in the approach I was taking in that patch.
Comment 22 Dão Gottwald [:dao] 2011-09-21 15:47:47 PDT
What are the odds that a followup bug to fix the DOM structure for this particular case, without a complete rewrite, would get done? I'm not familiar with this code, so I have no idea how involved this would be. I realize you initially dismissed this option -- I'd like to understand why.
Comment 23 Kyle Simpson 2011-09-21 16:10:09 PDT
(In reply to Dão Gottwald [:dao] from comment #22)
> What are the odds that a followup bug to fix the DOM structure for this
> particular case, without a complete rewrite, would get done? I'm not
> familiar with this code, so I have no idea how involved this would be. I
> realize you initially dismissed this option -- I'd like to understand why.

Fair question. I'd say the chances of such a follow-up bug (just fixing the markup nesting issues and related CSS) getting prioritized is much higher (at least in the forseeable future) than resuming bug #659710.

I never meant to suggest that I disagree (or dismiss) that what you've suggested is the (eventual) correct fix. I do agree. I just was weary to open a can o' worms with a potentially hairy set of issues with ioBox (the offending code) just to fix a minor bug.

I felt like the temporary fix for *this* bug was the right approach, and that a follow-up (either my full-rewrite or some other more limited re-address) could (and should) address it properly.

The proper person to answer the priority question for such a follow-up is dcamp, I believe.
Comment 24 Dão Gottwald [:dao] 2011-09-21 16:22:05 PDT
Comment on attachment 558342 [details] [diff] [review]
updated from feedback, removed tests

> .docType {
>   position: absolute;
>+  /* position DOCTYPE element above/outside the "nodeBox" that contains it */

So please file a followup bug on properly moving .docType out of .htmlNodeBox and include the bug number in the comment you're adding here.
Comment 25 Kyle Simpson 2011-09-22 07:09:15 PDT
Created attachment 561726 [details] [diff] [review]
[in-fx-team] updated CSS comments per Dao

no code changes, just updated the comments in the CSS per Dao
Comment 26 Rob Campbell [:rc] (:robcee) 2011-09-22 08:36:49 PDT
Comment on attachment 561726 [details] [diff] [review]
[in-fx-team] updated CSS comments per Dao

https://hg.mozilla.org/integration/fx-team/rev/bf900e375add
Comment 27 Rob Campbell [:rc] (:robcee) 2011-09-23 15:45:10 PDT
https://hg.mozilla.org/mozilla-central/rev/bf900e375add

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