Hi, I found that current dillo uses twice the given margin-bottom: value. See attached margin_bottom.html. It is related to commented out code in textblock.cc. Attached patch tries to fix it while not breaking the test cases in http://www.dillo.org/test/img/ I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph? Cheers, Johannes
Johannes wrote:
I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph?
My first thought was that maybe it has something to do with cases like <div style="margin-top: 100px"> <div style="margin-top: 100px"> <img src="http://www.dillo.org/wearlab.png" style="margin-top: 100px"> </div> </div> where I was thinking those were all supposed to collapse. I haven't looked into the spec at all yet, but we get a total of 300px on dillo, and firefox has the divs collapse and the img margin is separate (maybe a block/inline distinction?).
On Fri, Aug 26, 2011 at 04:21:04PM +0000, corvid wrote:
Johannes wrote:
I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph?
My first thought was that maybe it has something to do with cases like
<div style="margin-top: 100px"> <div style="margin-top: 100px"> <img src="http://www.dillo.org/wearlab.png" style="margin-top: 100px"> </div> </div>
where I was thinking those were all supposed to collapse. I haven't looked into the spec at all yet, but we get a total of 300px on dillo, and firefox has the divs collapse and the img margin is separate (maybe a block/inline distinction?).
seems so. changing it to <img src="http://www.dillo.org/wearlab.png" style="display: block; margin-top: 100px"> causes firefox to also collapse the img margin. I have margin collapsing also on my list.
On Fri, Aug 26, 2011 at 09:43:39PM +0200, Johannes Hofmann wrote:
On Fri, Aug 26, 2011 at 04:21:04PM +0000, corvid wrote:
Johannes wrote:
I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph?
My first thought was that maybe it has something to do with cases like
<div style="margin-top: 100px"> <div style="margin-top: 100px"> <img src="http://www.dillo.org/wearlab.png" style="margin-top: 100px"> </div> </div>
where I was thinking those were all supposed to collapse. I haven't looked into the spec at all yet, but we get a total of 300px on dillo, and firefox has the divs collapse and the img margin is separate (maybe a block/inline distinction?).
seems so. changing it to <img src="http://www.dillo.org/wearlab.png" style="display: block; margin-top: 100px"> causes firefox to also collapse the img margin. I have margin collapsing also on my list.
And on the textblock side, it seems to render just as before with the patch. Johannes, please decide on whether to apply it before the release. I'll try to pack RC2 this Monday. -- Cheers Jorge.-
On Fri, Aug 26, 2011 at 04:48:46PM -0300, Jorge Arellano Cid wrote:
On Fri, Aug 26, 2011 at 09:43:39PM +0200, Johannes Hofmann wrote:
On Fri, Aug 26, 2011 at 04:21:04PM +0000, corvid wrote:
Johannes wrote:
I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph?
My first thought was that maybe it has something to do with cases like
<div style="margin-top: 100px"> <div style="margin-top: 100px"> <img src="http://www.dillo.org/wearlab.png" style="margin-top: 100px"> </div> </div>
where I was thinking those were all supposed to collapse. I haven't looked into the spec at all yet, but we get a total of 300px on dillo, and firefox has the divs collapse and the img margin is separate (maybe a block/inline distinction?).
seems so. changing it to <img src="http://www.dillo.org/wearlab.png" style="display: block; margin-top: 100px"> causes firefox to also collapse the img margin. I have margin collapsing also on my list.
And on the textblock side, it seems to render just as before with the patch.
Johannes, please decide on whether to apply it before the release. I'll try to pack RC2 this Monday.
Committed, including some margin collapsing.
On Fri, Aug 26, 2011 at 10:20:43AM +0200, Johannes Hofmann wrote:
Hi,
I found that current dillo uses twice the given margin-bottom: value. See attached margin_bottom.html. It is related to commented out code in textblock.cc. Attached patch tries to fix it while not breaking the test cases in http://www.dillo.org/test/img/ I'm not 100% sure about removing the "if" in Textblock::wordWrap(), but why shouldn't a widget's top margin count in the first line of a paragraph?
AFAIR there was the "line1" offset stuff (which seem not to apply here because it's horizontal), and the handOverBreak() code which dealt with spacing between textblocks. I'm for applying the patch, but would give first a second look to handOverBreak() just to be sure. BTW, after a quick review it looks weird, as the value seems inherited (copied from previous stack object) most of the time... -- Cheers Jorge.-
participants (4)
-
corvid＠lavabit.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
johannes.hofmann＠gmx.de