Closed Bug 382721 Opened 18 years ago Closed 8 years ago

Dotted/dashed border-radiused corners are rendered as solid (no border-style support)

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
relnote-firefox --- 50+
firefox50 --- fixed

People

(Reporter: netrolller.3d, Assigned: arai)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, testcase)

Attachments

(32 files, 42 obsolete files)

(deleted), text/html
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/jpeg
Details
(deleted), image/jpeg
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), image/png
Details
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), image/png
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), text/html
Details
(deleted), image/png
Details
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
arai
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
arai
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), patch
jrmuizel
: review+
Details | Diff | Splinter Review
(deleted), image/jpeg
Details
Attached file Testcase, dotted circle is rendered as solid. (obsolete) (deleted) —
QUOTE(Vladimir Vukicevic, 2007-05-27 21:16:10 PDT, Bug 368247 comment #30): "-moz-border-radius: 100% will not result in a dotted/dashed circle, because the entire radius is considered a "corner". The latter (well, both) could be fixed at some later time if someone's bored." This bug is looking for "bored" people who could fix that. Testcase is attached.
Flags: blocking1.9?
Blocks: 368247
Keywords: regression, testcase
Summary: Dotted -moz-border-radiused corners are rendered as solid → Dotted -moz-border-radiused corners are rendered as solid (regression from bug 368247)
This is not a regression; Firefox 2 ignores dotted/dashed on borders with border-radius.
Assignee: vladimir → nobody
Flags: blocking1.9? → blocking1.9-
Keywords: regression
Changing summary because this is not a regression. Note however that the first approach of bug 368247 would have fixed this.
Summary: Dotted -moz-border-radiused corners are rendered as solid (regression from bug 368247) → Dotted -moz-border-radiused corners are rendered as solid
Blocks: 13944
I've had both bugs in my votes for a while, and I've just noticed this now... but isn't this bug a dupe of bug 13944?
Bug 13944 is essentially fixed now - it was about -moz-border-radius completely disabling dotted/dashed borders. This bug is about only the corners being solid.
Blocks: 468835
Summary: Dotted -moz-border-radiused corners are rendered as solid → Dotted/dashed -moz-border-radiused corners are rendered as solid
Flags: wanted1.9.2?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
This works on the new IE9 preview. Does it count as an "IE parity" blocker? It also works on Opera 10.5, and does not work in Safari 4 and Chrome 4.
It should, in my opinion, be marked as parity blockers, yes. Also, what are the actual issue(s) that makes fixing this hard?
(In reply to comment #8) > what are the actual issue(s) that makes fixing this hard? We need an algorithm to place the dots and dashes around the curve, and smoothly scale their sizes, and so on. We don't have one. Several people have tried and failed to come up with one.
I drew up a math problem to model this, here: http://fantasai.inkedblade.net/style/discuss/border-transitions/find-the-dots I posted it to a bunch of my friends, and one of them came up with a Python script and some comments. I haven't looked it over, but here's his comment: ====== Message from Michael O'Kelly ====== See pretty pictures: http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3a.png http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3b.png http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3c.png Code: http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3.py First finds a chain of circles assuming m=0, then uses the resulting overlap to calculate an optimal m. Inner loops mostly use rapidly-converging searches, along the lines of Victor Shnayder's proposal. They could be replaced with algebraic solutions (e.g. Jon Snitow's) for dramatic speedup. This code doesn't make any assumptions about the input curves--they could be ellipses or anything sufficiently smooth. ====== End Message ====== The key thing to note is that the centers of the circles don't lie on an ellipse: http://sharing.highlightcam.com.s3.amazonaws.com/ellipses2.png I believe James Socol has some of the actual math worked out, but he says it's very messy and doesn't have a full solution. Maybe that helps, maybe not, but I thought I'd post it here. :)
I wrote some tests and in all web engines the result is little strange: http://ikilote.net/Programmation/CSS/Test/borders_radius_1.htm
(In reply to comment #11) > I wrote some tests and in all web engines the result is little strange: > http://ikilote.net/Programmation/CSS/Test/borders_radius_1.htm That doesn't surprise me. Would you be willing to draw, with SVG or whatever, suggested renderings of each of those test boxes? That would be quite helpful. Be aware of bug 19963, which will also cause poor rendering of these examples - probably it should be a dependency, come to think.
Depends on: 19963
SVG like this ? : http://ikilote.net/Programmation/CSS/Test/borders_radius_1_test_1.svg I draw this (box1, 2, 5 & 6) rapidly... for example.
Just for the record, WebKit is currently working on this problem. See https://bugs.webkit.org/show_bug.cgi?id=9197 and http://trac.webkit.org/changeset/62035
Summary: Dotted/dashed -moz-border-radiused corners are rendered as solid → Dotted/dashed border-radiused corners are rendered as solid
Depends on: 652650
Benjamin, John, please do not post comments in bugs unless they are actively contributing towards fixing the bug. It clutters the bug report and unnecessarily spams everyone on the CC list. See https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Support for -moz-border-radius has been removed starting with Firefox 13. Updating the testcase and hiding invalid ones. Sorry for the spam.
Attachment #266827 - Attachment is obsolete: true
Attachment #369916 - Attachment is obsolete: true
When the border-radius is 50%, making a circle the border is rendered as solid
Depends on: 995677
(In reply to fantasai from comment #10) > http://fantasai.inkedblade.net/style/discuss/border-transitions/find-the-dots This first link is still active, but unfortunately > ====== Message from Michael O'Kelly ====== > http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3a.png > http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3b.png > http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3c.png > http://sharing.highlightcam.com.s3.amazonaws.com/ellipses3.py > http://sharing.highlightcam.com.s3.amazonaws.com/ellipses2.png are all gone now. Did anyone make copies to attach here to this bug, especially of the Python code? > I believe James Socol has some of the actual math worked out, but he says > it's very messy and doesn't have a full solution. It might be helpful to get a hold on that draft as well, even if not fully developed. Someone may have an idea where to go from here. But then, all of this was in 2010 and may be lost by now. :-\
Patt, this wasn't the kind of feedback I wanted to solicit with my comment #33, and not really helpful towards solving this issue. There are plenty of old (and older) bugs here. The point is that someone will have to come up with an approach to smoothly connect straight and curved parts of the border, either conceptually or in some pseudo or scripted code, and eventually as a patch.
Why not take a look at how Google solved it in Chromium? No point in reinventing the wheel.
Summary: Dotted/dashed border-radiused corners are rendered as solid → Dotted/dashed border-radiused corners are rendered as solid (no border-style support)
WRT comment #36 - Elbart, I'd just as soon not do Chrome's rendering 'cause it is kinda unattractive. Compare http://m8y.org/tmp/testcase375.xhtml in IE9/10/11 vs Chrome. Has a notch cut out of it. I think IE is intelligently adjusting the dash size or something.
(In reply to rsx11m from comment #35) > The point is that someone will have to come up > with an approach to smoothly connect straight and curved parts of the > border, either conceptually or in some pseudo or scripted code, and > eventually as a patch. Any support is better than none. Do we have to aim at "smooth" solution ? See Chrome example. It's not perfect but it is usable.
What don't you like about chrome's rendering? http://m8y.org/tmp/testcase375.xhtml is marginal. In IE dashed borders in real life cases (like text input borders) seem more like dotted.
Well, what I clearly don't like about chrome's rendering is it causes gaps. The circles is one example, but I don't see that as "marginal" IE's is maybe not ideal because they might allow the dashes to become too small, but that doesn't seem intrinsic to avoiding gaps. That said, if it isn't convenient to do something similar to IE's approach and copying the code is more convenient, by all means do chrome's. Was just an opinion that IE's is more attractive and and handles more cases nicely.
On http://forums.mozillazine.org/viewtopic.php?f=9&t=2834003 it was suggested that I post my solution proposal here. As I don't know how to post a document here, I try to copy the entire text here, hoping that the layout will not be disturbed too much: Suggestions for solving the dotted curves problem Unlike Chrome, Internet Explorer, Opera and Safari, Firefox v29 still renders dotted curves (e.g. rounded corners of boxes) as solid lines. I complained about this at http://forums.mozillazine.org/viewtopic.php?f=9&t=2834003. However, they suggested that I post my ideas about a possible solution here. 1. The problem When a box is given a dotted border, the straight border sections are correctly dotted. However, when the corners are given a radius, the curved parts are rendered solid instead. This is said to be due to a lack of a good rendering algorithm for dotted curves. I don't have an algorithm either, but I do have ideas for how to possibly find a solution. 2. Suggestions for solutions a. For the time being, I would rather not bother too much about accuracy. Anything is better than the cur-rent situation. Also, a box is not a stand-alone item; it will always be part of a page that contains more information, which will distract much of the reader’s from tiny inaccuracies – but obviously not from the big ones we have now. At a later stage, the solution might be further improved, if need be. b. The exact dot size is more important than the exact dot spacing. The latter may be varied slightly in order to make ‘the ends meet’ correctly. c. There is no need that the straight sections begin and end exactly at the centre of a dot! If they did, a box with four curved corners would need exact ‘fitting’ at eight positions – two for each curved corner. This is not necessary. It would be quite acceptable if all dots flow continuously along the border until the ends meet at one point. 3. Suggestions for algorithms I would describe the lines and curves in terms of analytical geometry. Dots will be placed on ‘carrier paths’, which may be any desired shape, and will normally be invisible. When one dot has been positioned, the next dot position can be calculated by moving along the carrier line. Depending on the situation, it may be on the same carrier path section, or on the next one. This is repeated until either the line ends, or the starting point is reached. (I don’t know if curves crossing itself – e.g. a lemniscate – are possible. If so, then be sure that the crossing point will be passed twice before the process is considered complete. If need be, fine-tuning – having only one dot at the crossing point – can be solved at a later stage.) For a curved carrier path section, there are two ways to calculate the next dot position: a. Along the curve itself: Walk along this circle either over an angle of rotation equal to the spacing needed divided by the border radius (if the curve is a circle), or in small steps until the required spacing is reached. b. Along the secant line to the next point: Draw a circle, centred at the current dot position, and with a ra-dius equal to the spacing needed. The next point is at the intersection of this circle and the carrier path. Method a may result in a visually slightly smaller dot spacing, as the (invisible) actual carrying line is circu-lar, whereas in method b it is straight. I don’t expect this to be a problem; or otherwise it may be simply compensated by a tiny increase in the dot spacing in a later version. At this moment, a basic solution is ur-gently needed; if need be, fine-tuning may be achieved in a later version. 4. Fine-tuning the result First, calculate the actual circumference length, without actually placing the dots. Then divide this circumference by the sum of the required dot size and dot spacing. If the result is not an integer number, then round it to the nearest integer. This rounded result is the actual number of dots needed. The circumference divided by this number of dots produces the actual sum of dot size and dot spacing needed. The dots can then be placed as described in section 3 above, using this modified ‘dot frequency’ (= dot size plus dot spacing). If this fine-tuning is a problem (which I can hardly believe), this fine-tuning might be delayed to a later version. It is too bad that for more than a decade or so this problem has persisted in a browser that has a reputation of being the best W3C-compliant browser available. This problem should be resolved at least basically as soon as possible. H. Hahn
Can you provide a patch (see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide for instructions), or failing that provide an implementation of the algorithm that is as close to being integrated into the source as possible? You may be able to find the current code at dxr.mozilla.org or mxr.mozilla.org. Thanks!
I'm afraid I cannot. I never developed browsers or anything alike. It would take far too much time to study dig up how it currently works or how to describe it in more detail so that you could use it as a basis for further work on it. If there are details in my text that are not clear, then you may of course ask me for further explanation. I would rather not dive into coding issues myself. I just described what I think is a logical approach to the problem. After all, if all other browsers did solve the problem, it cannot be that complicated? Anyway, I would emphasize again that fine-tuning is not the most urgent problem. A basic solution would do for the time being.
The testcases in attachment 673358 [details] are very simple and the most common cases. While fixing this bug, we should keep an eye on more complex use cases, like diffent radius and styles in one box and may be fix them later in a further step.
@sjw@gmx.ch: Different radiuses should not be more complicated. Just "follow the curve", whatever the radius at the current section. It does not even need to be circular. Start with a temporary solid line; its centreline will be the carrier path. When all dots are positioned, the centreline may be removed or made invisible.
One more point: Firefox already correctly rotates objects if asked to do so. Apparently it can rotate an object along a specified centre point, over a specified angle. That is just what must be done to "follow a curve": begin to move straight ahead over a very small distance delta (x), and then "rotate" the last-drawn "delta (x)" by its starting point just as far as needed to get back on the curve. Repeat this until the sum of all delta (x) steps equals the required do spacing. Then drop the next dot. Etcetera.
Attached image newtab_dashed.png (deleted) —
As of Firefox 33, about:newtab is using a rounded dash-border to signal an empty tile-space. Thus this bug has been made more obvious to users.
Even an imperfect algorithm would be better than nothing at this point.
(In reply to Elbart from comment #48) > As of Firefox 33, about:newtab is using a rounded dash-border to signal an > empty tile-space. Thus this bug has been made more obvious to users. Though this is a platform bug, putting it on the backlog since it affects Frontend.
Flags: firefox-backlog?
We don't use firefox-backlog for that kind of "Firefox wanted" tracking.
Flags: firefox-backlog? → firefox-backlog-
Attached image outline_customization.png (deleted) —
For the sake of completeness: outline is also affected, as seen in the placeholders in the right panel of the Customization-window. http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/customizeMode.inc.css?rev=3adeab7e472d#22
To everyone who is saying the "Just follow Chrome/ium", I say "Please, NO!" For dotted borders, Chrome renders ugly square bits. Although for 1px thick borders, most of the users would not notice, but designers would. And, if the borders get any thicker than 2px, it is apparent to everyone. And square bits look fugly to say it lightly. Any updates on how far we are into this? If a perfect solution is not possible at the moment, perhaps a quick solution would be OK, perhaps it can be improved with feedback in the FF developer edition rather than not implementing anything.
Agreed on prior comment. While Firefox is outright buggy, Chrome is the most unattractive. Safari is a bit better than Chrome, and IE seems to do the best at spacing and rendering dots/dashes.
I have seen several Firefox "updates" coming along during the last half year or so, but often I was wondering what was the actual urgency of the problems they solved. I am glad to see now that more and more people agree that the dotted-curve problem is getting really urgent. The way it is now, is a real shame for what is considered the most reliable browser around. Now who is going to solve it? Once again, not everybody writing here is sufficiently familiar with ins and outs of the Firefox innards. So not everyone of us is able to actually solve the problem. But there must be people who are -- it is they who produce all those tiny improvements we do get. So why not tackle this ugly-curve problem as soon as possible?
There's an easy way to make it render fine for the majority of use cases without having to implement something complicated. If the box has equal width on all corners, we could draw the rounded rectangle with a dotted/dashed pen, this is easy. Otherwise we resort to the current painting. People rarely use different widths for the corners a lot in the real world, even if they want, they probably don't need to.
Here is a WIP patch (just for record), there should be a lot of space to improve accuracy/performance, and cleanup, but at least it works in most cases. I'll post the some testcases and rendering result.
Assignee: nobody → arai.unmht
Attached file static test for various width (obsolete) (deleted) —
Attached file controllable test (obsolete) (deleted) —
Just noticed following note. https://drafts.csswg.org/css-backgrounds-3/#border-style > Note: There is no control over the spacing of the dots and dashes, > nor over the length of the dashes. Implementations are encouraged to > choose a spacing that makes the corners symmetrical. If we tweak the spacing of each edge to make them symmetric, corner implementation will become simpler :) I'll do it first, then will bring some things to discuss.
Several improvement from previous one. * variable overlap for dotted * variable dash length for dashed * symmetric border side * merge dotted+dotted corner with same width and no radius into single dot
Attachment #8636535 - Attachment is obsolete: true
Attached image result of WIP 2 patch (obsolete) (deleted) —
Attached image Rough algorithm used by the WIP 2 path (deleted) —
Attached image describes the way the patch uses. There're several issues for now. One of the big issue is the performance. Currently there're nested binary seaches, up to 5 loops nesting, so it's heavy when border-width is thin and curve is long. I guess some of them could be solved mathematically, by directly calculating the value instead of searching, and others may be improved by applying more linear approximation. Any hint is welcome :)
Attachment #8637436 - Attachment description: radius.jpg → Rough algorithm used by the WIP 2 path
Attached image Design issue on dotted corner (deleted) —
Then, here's a design issue in dotted corner In attached image, to fill the corner area with circles, there can be a circle which is smaller than circles at both ends, even if both border-width is same. Does anyone have any opinion on this, or have a reference image?
Have you looked at the patches from Bug 652650 ? They might have some usefuless.
From the look of your renderings this work will fix bug #19963 as well? Also, thank you for your these patches. I can’t comment on their technical proficiency, but I love the diagrams explaining the algorithm you’re using.
Attached image Possible improvement for dashed corner joint (obsolete) (deleted) —
(In reply to fantasai from comment #75) > Have you looked at the patches from Bug 652650 ? They might have some > usefuless. Thank you for letting me know that :D Yes, it helps a lot. So, you used elliptic arc instead of cubic bezier curve for calculation. It may be simpler than cubic bezier (yeah, this cubic bezier itself is the approximation of elliptic arc, but original elliptic arc will be suitable for some case), I'll try it. Also, the testcase there revealed that we need half offset for entire dashed side and corner. data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<style>%0D%0A p { height%3A 100px%3B width%3A 200px%3B border-width%3A 10px 20px%3B border-color%3A orange blue%3B border-radius%3A 60px%3B border-style%3A dashed%3B }%0D%0A<%2Fstyle>%0D%0A<p>and btw, there exists the code which calculates gradient color for corner, but it's not a required thing now, right? https://drafts.csswg.org/css-backgrounds-3/#changes-2009 > 9.6. Changes Since the 17 December 2009 Candidate Recommendation > Removal of recommendation to use gradients for color transitions when > ‘border-radius’ produces a curve.
(In reply to Robin Whittleton from comment #76) > From the look of your renderings this work will fix bug #19963 as well? This patch follows the rule in the bug only in dotted+dotted with same border-width and no border-radius case. "24 dotted" case in attachment 8637265 [details] is this case. For other cases that dot overlaps with other side, this patch joints them with solid corner (may have small radius). like "16 dotted 24 dotted" case in attachment 8637265 [details], which draws heart mark at every corner, that is half dots in both side, and solid rectangle in the corner. For example, the dotted+solid case is differ from attachment 323815 [details], with this patch, it's drawn as "16 solid 24 dotted" case.
Fixed the dashed and several minor rendering, and some cleanup. Not yet touched to performance.
Attachment #8637263 - Attachment is obsolete: true
Attached image result of WIP 3 patch (obsolete) (deleted) —
@Tooru thanks for your work so far. In the latest example I'm finding the simple dashed/dotted rendering to be strange. The corner dots or dashes appear smaller and/or offset inward by 1px. The dot at -90deg is also stretched rather than staying uniform while the spacing is adjusted instead. See: https://i.imgur.com/7cbmmLf.png Thanks!
Status: NEW → ASSIGNED
Attached image dot size and dash length issue (obsolete) (deleted) —
(In reply to Leon Sorokin from comment #81) > @Tooru thanks for your work so far. > > In the latest example I'm finding the simple dashed/dotted rendering to be > strange. The corner dots or dashes appear smaller and/or offset inward by > 1px. The dot at -90deg is also stretched rather than staying uniform while > the spacing is adjusted instead. > > See: https://i.imgur.com/7cbmmLf.png > > Thanks! Thank you for pointing them out. About the dotted style, the actual issue is that dots in straight side (including -90deg dot) are not perfect circles, but 1px wider, this is an existing bug (in other words, the circles in corner are rendered with expected size). Currently we're rendering it as dashed line with [0, 2 * borderWidth] pattern and round line cap (0 length line with round line cap in both side, and gap between them), it's rendered like that. We might have to find another way to render dotted side, to improve this. maybe render each circles separately? I'll try this, and check the performance. About dashed style, this patch is now calculating the width of dash/gap separately for side and corner. So side and corner may have different dash/gap width. Also, in attached case, left side has 1 px length which is rendered as solid border, since no dashed line fits there. this makes the segment in -90deg a bit longer. To improve them, I think we have to calculate dashed width and offset for side and corner at the same time (so, we shouldn't joint corner and side with half segment), at least for simple case, like all border has same width, and radii are large enough.
Addressed the dotted side's issue, and improved performance for some simple cases.
Attachment #8638919 - Attachment is obsolete: true
Attached image result of WIP 4 patch (obsolete) (deleted) —
(In reply to Tooru Fujisawa [:arai] from comment #77) > > btw, there exists the code which calculates gradient color for corner, but > it's not a required thing now, right? > > https://drafts.csswg.org/css-backgrounds-3/#changes-2009 > > 9.6. Changes Since the 17 December 2009 Candidate Recommendation > > Removal of recommendation to use gradients for color transitions when > > ‘border-radius’ produces a curve. It is not required, no, because nobody wanted to implement it. But I think if it looks better, then that's better, no?
(In reply to fantasai from comment #85) > (In reply to Tooru Fujisawa [:arai] from comment #77) > > > > btw, there exists the code which calculates gradient color for corner, but > > it's not a required thing now, right? > > > > https://drafts.csswg.org/css-backgrounds-3/#changes-2009 > > > 9.6. Changes Since the 17 December 2009 Candidate Recommendation > > > Removal of recommendation to use gradients for color transitions when > > > ‘border-radius’ produces a curve. > > It is not required, no, because nobody wanted to implement it. But I think > if it looks better, then that's better, no? Yeah, it might be better. But I'd like to leave it to another bug, since it also affects other styles.
WIP 4 looks much better, though the spacing is still somewhat biased towards the top & bottom edges: https://i.imgur.com/wXSIPkc.png
Attached file static test for various width (obsolete) (deleted) —
testcase used for WIP 4 result
Attachment #8636537 - Attachment is obsolete: true
(In reply to Leon Sorokin from comment #87) > WIP 4 looks much better, though the spacing is still somewhat biased towards > the top & bottom edges: https://i.imgur.com/wXSIPkc.png Thank you for pointing that out, the glitch was caused by floating point number handling and optimization for simple case didn't work (it still exists in semi-optimized case tho).
On Flame, displaying attachment 8640602 [details] takes about 5 seconds to redraw, after scroll. I guess this is not acceptable performance :/
Fixed the floating point number handling, and improved dotted overlap/dashed length finding algorithms, not they can find the best value almost within 8 iterations. This takes 3 or 4 seconds for each redraw on Flame, slightly faster than WIP 4, but still too slow. btw, is there any proper way to evaluate the rendering performance?
Attachment #8640231 - Attachment is obsolete: true
Attached file static test for various width (obsolete) (deleted) —
added 2 more test case with 0 border-width
Attachment #8640602 - Attachment is obsolete: true
Attached image result of WIP 5 patch (obsolete) (deleted) —
Attachment #8636539 - Attachment is obsolete: true
Attachment #8637265 - Attachment is obsolete: true
Attachment #8637818 - Attachment is obsolete: true
Attachment #8638920 - Attachment is obsolete: true
Attachment #8638960 - Attachment is obsolete: true
* improved search algorithms * fixed bug 19963 (I guess I need to split this patch into several parts later) * corner between dotted side and non-dotted side * corner between dotted sides with different border widths * now no *unintentional* heart mark * some minor optimization * cleanup
Attachment #8642053 - Attachment is obsolete: true
Attached file static test for various width (obsolete) (deleted) —
added more tests
Attachment #8642055 - Attachment is obsolete: true
Attached file controllable test (deleted) —
added more options
Attachment #8636538 - Attachment is obsolete: true
Attached image result of WIP 6 patch (obsolete) (deleted) —
so much testcases :) corner between moz-border-colors is not supported
Attachment #8640232 - Attachment is obsolete: true
Attachment #8642056 - Attachment is obsolete: true
can I get some feedback on this? Changes are following (I'll post some figures shortly): * Applied dotted/dashed to border radius corner + nsCSSBorderRenderer::DrawDashedCorner + nsCSSBorderRenderer::DrawDottedCornerSlow and DottedCornerFinder + nsCSSBorderRenderer::DrawDashedCornerSlow and DashedCornerFinder * Made dotted/dashed border side symmetric, by changing the spacing + nsCSSBorderRenderer::CalculateDashedOptions from nsCSSBorderRenderer::DrawDashedSide and nsCSSBorderRenderer::DrawDottedSide * Draw dotted side with separated circles, instead of dashed with ROUND + nsCSSBorderRenderer::DrawDottedSide * Made interaction between dotted side and other side better + SIDE_CLIP_RECTANGLE_CORNER and SIDE_CLIP_RECTANGLE_NO_CORNER in nsCSSBorderRenderer::GetSideClipSubPath + nsCSSBorderRenderer::IsCornerMergeable for dotted sides + start/end position calculation (nsCSSBorderRenderer::GetStraightBorderPoint) in nsCSSBorderRenderer::DrawDashedSide and nsCSSBorderRenderer::DrawDottedSide * Changed DASH_LENGTH from 3 to 2 rendering result seems to be much better with 2 * Added several utility functions for Corner/Side + GetHorizontalSide/GetVerticalSide + GetCWSide/GetCCWSide + GetCWCorner/GetCCWCorner here's try run result https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a20c3e08cf1 the failure in 461512-1.html seems to be almost same reason as dotted cannot be tested, so I think that part should also be commented out. then, I have some questions: 1. What would be the best way to test border-radius rendering? 2. What kind of performance test should I do? is there any upper bound for time taken by each redraw (especially with mobile device)?
Attachment #8644116 - Attachment is obsolete: true
Attachment #8647735 - Flags: feedback?(roc)
Attached image result of WIP 7 patch (obsolete) (deleted) —
Attachment #8644119 - Attachment is obsolete: true
Attached image DottedCornerFinder's behavior (deleted) —
Attached image DashedCornerFinder's behavior (deleted) —
This looks really amazingly impressive, and I'm excited about getting it landed! But it's going to be hard to review. The diagrams should be checked in alongside the code, and you should reference them in comments. (In reply to Tooru Fujisawa [:arai] from comment #98) > then, I have some questions: > 1. What would be the best way to test border-radius rendering? That's a very good question! One way to test this would be to write a mochitest that draws borders into a canvas --- e.g. by creating an SVG image containing a foreignobject containing some HTML+CSS to render: http://robert.ocallahan.org/2011/11/drawing-dom-content-to-canvas.html Then, for each rendered border, call getImageData and probe specific pixels to make sure they have the expected value. E.g. test that points definitely inside a dot or dash are drawn, and test that points definitely outside a dot or dash are not drawn. > 2. What kind of performance test should I do? > is there any upper bound for time taken by each redraw (especially with > mobile device)? It's very important that common cases of simple borders be fast. Maybe nothing you're changing here actually affects this. It's also very important that repeated drawing of the same border be fast. So you might want to look at the parts that are expensive and think about adding some caching. It would be extra-good if the caching is designed so that when corners are symmetrical (which they usually are) we can calculate and cache values for the top-left corner and use the cached values for the other corners.
(In reply to Tooru Fujisawa [:arai] from comment #99) > Created attachment 8647736 [details] > result of WIP 7 patch Impressive work. I've only looked briefly at this bug and your patch so might be completely misunderstanding things but I noticed that you're doing a lot of the analytical work on bezier curves. Is this easier than working with the ellipses directly?
Flags: needinfo?(arai.unmht)
Thank you roc and jrmuizel :) (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #103) > This looks really amazingly impressive, and I'm excited about getting it > landed! But it's going to be hard to review. > > The diagrams should be checked in alongside the code, and you should > reference them in comments. > > (In reply to Tooru Fujisawa [:arai] from comment #98) > > then, I have some questions: > > 1. What would be the best way to test border-radius rendering? > > That's a very good question! > > One way to test this would be to write a mochitest that draws borders into a > canvas --- e.g. by creating an SVG image containing a foreignobject > containing some HTML+CSS to render: > http://robert.ocallahan.org/2011/11/drawing-dom-content-to-canvas.html > Then, for each rendered border, call getImageData and probe specific pixels > to make sure they have the expected value. E.g. test that points definitely > inside a dot or dash are drawn, and test that points definitely outside a > dot or dash are not drawn. Thanks, now I understand the meaning of the mask images in bug 19963. So I can mask the rendered image and check the correctness. I'll try it :) > > 2. What kind of performance test should I do? > > is there any upper bound for time taken by each redraw (especially with > > mobile device)? > > It's very important that common cases of simple borders be fast. Maybe > nothing you're changing here actually affects this. > > It's also very important that repeated drawing of the same border be fast. > So you might want to look at the parts that are expensive and think about > adding some caching. It would be extra-good if the caching is designed so > that when corners are symmetrical (which they usually are) we can calculate > and cache values for the top-left corner and use the cached values for the > other corners. I'll prepare cache for overlap and dash length calculation in finder classes. Those parts take so much time in the calculation, and it should be same for symmetrical borders. (In reply to Jeff Muizelaar [:jrmuizel] from comment #104) > (In reply to Tooru Fujisawa [:arai] from comment #99) > > Created attachment 8647736 [details] > > result of WIP 7 patch > > Impressive work. I've only looked briefly at this bug and your patch so > might be completely misunderstanding things but I noticed that you're doing > a lot of the analytical work on bezier curves. Is this easier than working > with the ellipses directly? There are some reasons why I use bezier curve in many places: 1. In most case, I need to handle the curve as parametric curve like x = Fx(t) and y = Fy(t), and in that case I think bezier curve is less-expensive than elliptic arc. iiuc, elliptic arc needs sin/cos or sqrt or something like that, doesn't it? 2. in dashed border, finally we need to fill each segment with the region consisted with bezier curves and straight lines (am I correct?), so we need control points for them, and in that case calculating everything with bezier curve will be straightforward. 3. I cannot find a good estimation for elliptic arc's length that is better than bezier curve's length for given segment. 4. Just because I'm not familiar with elliptic arc. so, if you know better solution with elliptic arc, let me know that ;) I use elliptic arc when it seems to be simpler, like when calculating whole arc's length (GetQuarterEllipticArcLength), solving equation with x and y (CalculateDistanceToEllipticArc), etc.
Flags: needinfo?(arai.unmht)
Changes: * Added several figures as comments * Added cache for best overlap and dash length for given corner parameters Now it takes up to 1 or 2 seconds for each redraw on Flame * Added reftests It renders dashed/dotted borders, overlays two masks: not-filled region with filled-color, and not-unfilled region with unfilled-color, and compares them with full-filled/fill-unfilled boxes * Always calculate best dashed length for dashed side (even with width <= 2.0) * Separated into several files * Changed DASH_LENGTH back to 3 (looks like [1,3] range is good enough) * Use arc length instead of direct distance in dashed corner calculation * If dot may overflow from outer rect, don't draw it (will post in next comment) * If corner between dotted sides is merged into one dot and sides have same color, draw the dot with single path instead of 2 half circles * Applied mozilla coding style for newly added code * Fixed dash offset in simple case for dotted border in nsCSSBorderRenderer::DrawBorders there strokeOptions.mDashOffset is set to 0.5f but it should be 0.5f * mBorderWidths[0], former results in 3 dots filled 1 dot gap in 2px dotted border because of AntialiasMode::NONE * Removed unused method declaration SetupStrokeStyle * Added some missing #include and using namespace. Questions: 1. Where should I store cache data? Currently cache is allocates as a global variable, but I think that data should be associated with document. 2. What would be the best data structure to store cache data? Is there any good data structure or existing template that provides O(log(n)) search and I can remove old data? In BorderCache.h, I use 2 data structures, a ring map and hash map (using either one depending on #ifdef), former is O(n) search but it removes oldest data when adding new one, latter one cannot remove oldest data.
Attachment #8647735 - Attachment is obsolete: true
Attachment #8647735 - Flags: feedback?(roc)
Attachment #8654682 - Flags: feedback?(roc)
Attached file static test for various width (deleted) —
Attachment #8644117 - Attachment is obsolete: true
Attached image result of WIP 8 patch (obsolete) (deleted) —
Attachment #8647736 - Attachment is obsolete: true
One more question, what would be the expected rendering in attached image? If width or height is smaller than border-width, there's no space to draw a circle with border-width/2 radius in that side. In WIP 8 patch, I just skip rendering that circle, but it results in empty border.
When the border-radius curve extends into the opposite side, the side and corner overlaps and the side overflows from the curve's revion. https://drafts.csswg.org/css-backgrounds-3/#corner-shaping I'd like to leave them to another bug since it happens also with solid border. We'll need to handle that case specially.
looks like the spacing uniformity in the simple dotted case has regressed since WIP7 https://i.imgur.com/i2KCztK.png
(In reply to Leon Sorokin from comment #111) > looks like the spacing uniformity in the simple dotted case has regressed > since WIP7 > > https://i.imgur.com/i2KCztK.png Thank you for pointing that out :) Yeah, in that one, previous rendering should be better, I'll check them. however, I don't do anything for "uniformity", each side and corner is rendered separately and spacing is also calculated separately, to place dots at the end of each side, and make corner starts and ends with dots (otherwise corner rendering becomes more difficult), so spacing uniformity is not guaranteed now. (of course, I'll try choosing better spacing for given length)
oops, that was just a typo :P > static bool > IsSingleCurve(Float aMinR, Float aMaxR, > Float aMinBorderRadius, Float aMaxBorderRadius) > { > return aMinR > 0.0f && > aMinBorderRadius > aMaxR * 4.0f && >- aMinBorderRadius / aMaxBorderRadius < 0.5f; >+ aMinBorderRadius / aMaxBorderRadius > 0.5f; > } no other changes from WIP 8 in Comment #106. re-posting questions just in case: 1. Where should I store cache data? Currently cache is allocates as a global variable, but I think that data should be associated with document. 2. What would be the best data structure to store cache data? Is there any good data structure or existing template that provides O(log(n)) search and I can remove old data? In BorderCache.h, I use 2 data structures, a ring map and hash map (using either one depending on #ifdef), former is O(n) search but it removes oldest data when adding new one, latter one cannot remove oldest data.
Attachment #8654682 - Attachment is obsolete: true
Attachment #8654682 - Flags: feedback?(roc)
Attachment #8655353 - Flags: feedback?(roc)
Attached image result of WIP 9 patch (obsolete) (deleted) —
Attachment #8654684 - Attachment is obsolete: true
Comment on attachment 8655353 [details] [diff] [review] (WIP 9) Support dotted/dashed border-radiused corners. Review of attachment 8655353 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/BezierUtils.h @@ +48,5 @@ > +mozilla::gfx::Float CalculateDistanceToEllipticArc(const mozilla::gfx::Point& P, > + const mozilla::gfx::Point& normal, > + const mozilla::gfx::Point& origin, > + mozilla::gfx::Float width, > + mozilla::gfx::Float height); Add comments explaining all these functions, especially where you have "2", "A" and "B" versions. ::: layout/base/BorderCache.h @@ +13,5 @@ > + > +namespace mozilla { > +// Cache for best overlap and best dashLength. > + > +struct fourFloats FourFloats @@ +96,5 @@ > +}; > + > +#else > + > +class fourFloatsHashKey : public PLDHashEntryHdr FourFloatsHashKey
(In reply to Tooru Fujisawa [:arai] from comment #113) > re-posting questions just in case: > 1. Where should I store cache data? > Currently cache is allocates as a global variable, but I think that > data should be associated with document. I think it should be global. It's common to have multiple documents with the same styles styles --- multiple pages open from the same site. > 2. What would be the best data structure to store cache data? > Is there any good data structure or existing template that provides > O(log(n)) search and I can remove old data? > In BorderCache.h, I use 2 data structures, a ring map and hash map > (using either one depending on #ifdef), > former is O(n) search but it removes oldest data when adding new one, > latter one cannot remove oldest data. A simple approach that should be OK is to use a hashmap and clear the entire cache when it gets full.
Attached patch Part 0: Add missing includes and namespaces. (obsolete) (deleted) — Splinter Review
Thank you for your feedback :) Now I split patches into 6 parts, for each change. First, I bumped into some errors when adding files to UNIFIED_SOURCES, and there are some more errors while testing non-unified build in layout/base/. So, as Part 0, I'd fix them. [layout/base/AccessibleCaretEventHub.cpp] > +#include "nsCanvasFrame.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/AccessibleCaretEventHub.cpp#391 > !aPresShell->GetCanvasFrame()->GetCustomContentContainer()) { nsPresShell::GetCanvasFrame returns nsCanvasFrame*. [layout/base/AccessibleCaretManager.cpp] > +#include "mozilla/Preferences.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/AccessibleCaretManager.cpp#863 > Preferences::AddUintVarCache(&caretTimeoutMs, Preferences is used here. > +#include "nsContainerFrame.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/AccessibleCaretManager.cpp#536 > focusableFrame = focusableFrame->GetParent(); nsIFrame::GetParent returns nsContainerFrame. [layout/base/ActiveLayerTracker.cpp] > +using namespace dom; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/ActiveLayerTracker.cpp?offset=0#381 > KeyframeEffectReadOnly* effect = mozilla::dom:: is omitted. [layout/base/MobileViewportManager.cpp] > +#include "gfxPrefs.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp?offset=100#281 > if (gfxPrefs::APZAllowZooming()) { gfxPrefs is used here. > +#include "nsIDOMEvent.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp#88 > event->GetType(type); nsIDOMEvent is used here. > +#include "nsIFrame.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp?offset=0#213 > if (!nsLayoutUtils::GetDisplayPort(root->GetContent(), nullptr)) { root is nsIFrame*. > +#include "nsIScrollableFrame.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp?offset=200#217 > nsIScrollableFrame* scrollable = do_QueryFrame(root); 'do_QueryFrame::operator nsIScrollableFrame *<nsIScrollableFrame>' is requested here. > +#include "nsLayoutUtils.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp#123 > LayoutDeviceToLayerScale res(nsLayoutUtils::GetResolution(mPresShell)); nsLayoutUtils is used here. > +#include "nsPresContext.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp#121 > CSSToLayoutDeviceScale cssToDev((float)nsPresContext::AppUnitsPerCSSPixel() nsPresContext is used here. > +#include "UnitTransforms.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/MobileViewportManager.cpp#138 > CSSToParentLayerScale zoom = ViewTargetAs<ParentLayerPixel>(defaultZoom, ViewTargetAs is used here. [layout/base/RestyleManager.cpp] > +using namespace dom; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/RestyleManager.cpp#2956 > FlattenedChildIterator it(aElement); mozilla::dom:: is omitted. [layout/base/TouchCaret.cpp] > +#include "nsDocShell.h" > docShell->AddWeakScrollObserver(this); nsDocShell is used here. [layout/base/TouchCaret.h] > +class nsDocShell; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchCaret.h#294 > WeakPtr<nsDocShell> mDocShell; nsDocShell is used here. [layout/base/TouchManager.cpp] > +#include "mozilla/TouchEvents.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.cpp#59 > WidgetTouchEvent event(true, NS_TOUCH_END, widget); WidgetTouchEvent is used here. > +#include "nsIFrame.h" > +#include "nsView.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.cpp#57 > nsCOMPtr<nsIWidget> widget = frame->GetView()->GetNearestWidget(&pt); frame is nsIFrame*, and GetView() returns nsView*. > +using namespace mozilla; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.cpp#12 > nsRefPtrHashtable<nsUint32HashKey, dom::Touch>* TouchManager::gCaptureTouchList; mozilla:: is omitted. > +using namespace mozilla::dom; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.cpp#214 > nsCOMPtr<EventTarget> targetPtr = oldTouch->mTarget; mozilla::dom:: is omitted. [layout/base/TouchManager.h] > +#include "mozilla/BasicEvents.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.h#27 > bool PreHandleEvent(mozilla::WidgetEvent* aEvent, WidgetEvent is used here > +#include "mozilla/dom/Touch.h" > +#include "nsRefPtrHashtable.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/TouchManager.h#34 > static nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Touch>* gCaptureTouchList; nsRefPtrHashtable and mozilla::dom::Touch are used here [layout/base/ZoomConstraintsClient.cpp] > +#include "gfxPrefs.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/ZoomConstraintsClient.cpp?offset=0#166 > constraints.mAllowZoom = aViewportInfo.IsZoomAllowed() && gfxPrefs::APZAllowZooming(); gfxPrefs is used here > +#include "mozilla/Preferences.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/ZoomConstraintsClient.cpp#80 > Preferences::RemoveObserver(this, "browser.ui.zoom.force-user-scalable"); Preferences is used here. [layout/base/nsLayoutDebugger.cpp] > +#include "nsAttrValue.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/nsLayoutDebugger.cpp#158 > content->GetClasses()->ToString(tmp); GetClasses() returns nsAttrValue*. [layout/base/nsLayoutUtils.cpp] > +#include "nsTextFragment.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/base/nsLayoutUtils.cpp#8640 > GetText()->AppendTo(aResult, offset, length); GetText() returns nsTextFragment* [layout/base/nsPresShell.cpp] > +#include "gfxPrefs.h" https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#986 > if (gfxPrefs::MetaViewportEnabled() || gfxPrefs::APZAllowZooming()) { gfxPrefs is used here [layout/generic/nsCanvasFrame.h] > +#include "mozilla/dom/Element.h" https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/generic/nsCanvasFrame.h#34 > explicit nsCanvasFrame(nsStyleContext* aContext) nsCOMPtr<mozilla::dom::Element>::assert_validity is requested in nsCanvasFrame ctor. [layout/style/nsStyleStruct.h] > +class nsStyleContext; https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/layout/style/nsStyleStruct.h#1369 > nsStyleContext* aContext) const; nsStyleContext needs declaration.
Attachment #8655353 - Attachment is obsolete: true
Attachment #8655353 - Flags: feedback?(roc)
Attachment #8659312 - Flags: review?(roc)
Part 1 fixes the rendering result of 2px dotted border with no radius. Because of AntialiasMode::NONE is used and mDashOffset is 0.5, there 3px filled and 1px spacing, on OSX. mDashOffset should be relative to border width. I'll post different in next comment. I don't add test for this because the rendering result still depends on OS, especially the at the corner.
Attachment #8659314 - Flags: review?(roc)
Attached image Rendering result of 2px dotted border (deleted) —
Part 1 fixes attaches case.
Part 2 just makes BorderConsts.h, which has some definition used by several files (also files added by Part 4)
Attachment #8659317 - Flags: review?(roc)
Part 3 applies following changes: * Make spacing of dashed/dotted border variable, to make it symmetric * Disable or add fuzzy option to test which is affected by anti-alias caused by spacing change * Draw dots in dotted border with width>2px separately, to make it perfect circle * Make corner interaction between dotted sides, or dotted and other sides. * at corner between dotted sides with same width, dots are merged into single dot * at corner between dotted sides with different width, wider side draws the dot at corner * at corner between dotted side and other side, other side always draws the corner
Attachment #8659323 - Flags: review?(roc)
Part 4 adds support for border-radius with dotted/dashed. Now cache is implemented by hashmap on global, entries are cleared when 257-th cache is stored. Static test doesn't hit this, only 120 to 130 cache entries. I might have to investigate how much caches we need for real websites (do you know where it's used in practice?). Then, in WIP 9 patch, the cache didn't work correctly (the performance improvement comes from other changes, like parameter tweak), now it's fixed and the static test is shown almost smoothly on Flame, it takes 1 or 2 seconds on some heavy style, but I don't need to wait for rendering for other cases, while scrolling.
Attachment #8659324 - Flags: review?(roc)
Part 5 adds reftests for Part 3 and Part 4. Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6364a3081956
Attachment #8659325 - Flags: review?(roc)
Attached image Rendering result of static test (obsolete) (deleted) —
Attachment #8655355 - Attachment is obsolete: true
A couple of us in the Toronto office are going to try to take a look reviewing this during our lunch break.
Comment on attachment 8659323 [details] [diff] [review] Part 3: Improve spacing and corner interaction of dashed/dotted border. Review of attachment 8659323 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRenderingBorders.cpp @@ +666,5 @@ > > +Point > +nsCSSBorderRenderer::GetStraightBorderPoint(mozilla::css::Side aSide, > + mozilla::css::Corner aCorner, > + bool* aIsUnfilled) It would be nice to have a better description of what this method does and why you want to do it. @@ +764,5 @@ > + // |#########| > + // | ####### | > + // | ##### | > + // | | > + Float minimum = borderWidth * /*(1.0f + sqrt(2.0f)) / 2.0f*/ 1.5f ; Why are you approximating this as 1.5?
Also, if you can add a paragraph of text describing the overall approach you're taking in Part 3 and Part 4 that would be helpful in reviewing.
Thank you for reviewing :D (In reply to Jeff Muizelaar [:jrmuizel] from comment #126) > ::: layout/base/nsCSSRenderingBorders.cpp > @@ +666,5 @@ > > > > +Point > > +nsCSSBorderRenderer::GetStraightBorderPoint(mozilla::css::Side aSide, > > + mozilla::css::Corner aCorner, > > + bool* aIsUnfilled) > > It would be nice to have a better description of what this method does and > why you want to do it. (In reply to Jeff Muizelaar [:jrmuizel] from comment #127) > Also, if you can add a paragraph of text describing the overall approach > you're taking in Part 3 and Part 4 that would be helpful in reviewing. Sure, I'll add those comments, will post updated patches by weekend (hopefully tomorrow). > @@ +764,5 @@ > > + // |#########| > > + // | ####### | > > + // | ##### | > > + // | | > > + Float minimum = borderWidth * /*(1.0f + sqrt(2.0f)) / 2.0f*/ 1.5f ; > > Why are you approximating this as 1.5? Sorry, I forgot to remove old code and update comment. 1.5 is better than (1+sqrt(2))/2. (1+sqrt(2))/2 is the value that two dots contact. But we should make some gap between them even if radius is small (but larger than border-width/2). (see attached image) There will be better value than 1.5, but I don't have any idea now.
(In reply to Tooru Fujisawa [:arai] from comment #128) > > Sorry, I forgot to remove old code and update comment. 1.5 is better than > (1+sqrt(2))/2. > (1+sqrt(2))/2 is the value that two dots contact. But we should make some > gap between them even if radius is small (but larger than border-width/2). > (see attached image) > There will be better value than 1.5, but I don't have any idea now. It's worth adding a comment about this.
Added comments to GetStraightBorderPoint and DrawDashedOrDottedSide in Part 3, and DrawDashedOrDottedCorner in Part 4. Tell me if those comments doesn't make sense, or if some required parts are not described. Thanks! Also, fixed the minimum length to draw dash in corner/side in Part 3, there, minimum dash length was lower than 1 (that is too short dashed segment) in previous patch, but dash length should be in [1, 3] range. This affects to some testcase, so updated them in Part 5.
Attachment #8659323 - Attachment is obsolete: true
Attachment #8659323 - Flags: review?(roc)
Attachment #8662302 - Flags: review?(jmuizelaar)
Attachment #8659324 - Attachment is obsolete: true
Attachment #8659324 - Flags: review?(roc)
Attachment #8662303 - Flags: review?(jmuizelaar)
Attachment #8659325 - Attachment is obsolete: true
Attachment #8659325 - Flags: review?(roc)
Attachment #8662304 - Flags: review?(jmuizelaar)
Attached image Rendering result of static test (deleted) —
Attachment #8659326 - Attachment is obsolete: true
Tooru, do you think you could get some timings of the border drawing call before and after this code? That will give us a feeling for whether we need to worry about the performance of the new implementation.
Flags: needinfo?(arai.unmht)
Attached file Time taken by nsCSSBorderRenderer::DrawBorders (obsolete) (deleted) —
Here's the result of performance comparison :) (I'll post screenshot of patched rendering in next comment) Let me know if more cases or detailed data is needed.
Flags: needinfo?(arai.unmht)
(In reply to Tooru Fujisawa [:arai] from comment #136) > Created attachment 8664434 [details] > Rendering result of "Time taken by nsCSSBorderRenderer::DrawBorders" file That's great. If it's easy to do, it would be nice to separate out the Moz2D/drawing portion of the time from the nsCSSBorderRenderer::DrawBorders calculations.
Updated. in P1 and P2, 1st number (red) is the time taken by calculation, 2nd number (blue) is the time taken by drawing.
Attachment #8664431 - Attachment is obsolete: true
(In reply to Tooru Fujisawa [:arai] from comment #139) > Created attachment 8664480 [details] > Rendering result of "Time taken by nsCSSBorderRenderer::DrawBorders" file What OS was the timing done on?
Flags: needinfo?(arai.unmht)
oh, sorry I forgot to note the basic information. I tested on OSX 10.10.5, with 64bit build nightly, on iMac Late 2013 (2.9 GHz Intel Core i5, 16 GB 1600 MHz DDR3, NVIDIA GeForce GT 750M 1024 MB)
Flags: needinfo?(arai.unmht)
Sorry for the delay, I'm going to try and finish up the review (at least up to part 3) this week. Is it possible to get timing information for just the first part (Up to part 3) of the patch?
Thanks! Here's result with Part 1-3
Here's the rendering result with part 1-3 (corners are still rendered with solid stroke, even if they're merged)
Attachment #8662302 - Flags: review?(jmuizelaar) → review+
Thank you for reviewing :) Is there anything I can do here for now? (looks like same patch is still applicable and no rebase is needed)
[~OOT] Could somebody please install the patch and tell if this testcase with "border-style: dashed solid;" - lags not more than with "border-style: solid;". This is what I'd expect from the patch. > the testcase: https://bug780366.bmoattachments.org/attachment.cgi?id=8684574 Just compare the testcase with checkbox "border-style" enabled/disabled (you can also try animation)
To be clear, this patch does not handle the bug 780366's case, so the rendering result is still wrong and the performance of it won't be meaningful value for the case (I think we need totally different approach for the case, than current one). Attachment 8654686 [details] in comment #110 is the result for almost same situation. > lags not more than with "border-style: solid;". Here's results: without animation (10 samples for each) "solid" border: 288 [us] "dashed solid" border: 641 [us] with animation (200 samples for each) "solid" border: 307 [us] (min:273, max:364) "dashed solid" border: 729 [us] (min:688, max:964) Those should be in acceptable range.
Just noticed that there some more missing #include's added from last time. No other changes in other parts.
Attachment #8684600 - Flags: review?(jmuizelaar)
Attachment #8684600 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8662303 [details] [diff] [review] Part 4: Support dotted/dashed border-radiused corners. Review of attachment 8662303 [details] [diff] [review]: ----------------------------------------------------------------- I haven't looked at everything yet, but here are some preliminary comments. It might be worth packaging up Point[4] into a Curve or Bezier type. This will let you assign them instead of having to memcpy. ::: layout/base/BezierUtils.cpp @@ +260,5 @@ > +Float > +GetQuarterEllipticArcLength(Float a, Float b) > +{ > + Float A = a + b, S = a - b; > + Float A2 = pow(A, 2.0f), S2 = pow(S, 2.0f); It's more performant to write these as: A2 = A*A; S2 = S*S; @@ +262,5 @@ > +{ > + Float A = a + b, S = a - b; > + Float A2 = pow(A, 2.0f), S2 = pow(S, 2.0f); > + > + return M_PI * A * (3.00f * S2 / (A2 * (sqrt(-3.0f * S2 / A2 + 4.0f) + 10.0f)) + 1.0f) / 4.0f; Where does this formula come from? ::: layout/base/BezierUtils.h @@ +1,4 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ It would be nice if BezierUtils.cpp and BezierUtils.h could go in gfx/2d as that's a more likely place for people to look for this kind of thing. @@ +6,5 @@ > +#ifndef mozilla_BezierUtils_h_ > +#define mozilla_BezierUtils_h_ > + > +#include "mozilla/gfx/2D.h" > +#include "gfxRect.h" Is this include needed? ::: layout/base/DashedCornerFinder.cpp @@ +128,5 @@ > + > +DashedCornerFinder::Result > +DashedCornerFinder::Next(void) > +{ > + Float lastTo, lastTi, to, ti; I think calling these lastOuterT and outerT would be clearer than abbreviating down to a single letter. @@ +197,5 @@ > + Point innerSectionPoints[4]; > + SplitBezierB(tmp, mOuterPoints, lastTo); > + SplitBezierA(outerSectionPoints, tmp, ConvertRange(to, lastTo)); > + SplitBezierB(tmp, mInnerPoints, lastTi); > + SplitBezierA(innerSectionPoints, tmp, ConvertRange(to, lastTo)); Should this be? SplitBezierA(innerSectionPoints, tmp, ConvertRange(ti, lastTi)); instead of To? Would it be better to have a SplitBezier function that takes a t1 and t2? ::: layout/base/DashedCornerFinder.h @@ +175,5 @@ > + Point mLastPi; > + Float mLastTo; > + Float mLastTi; > + > + // Length for each segment, radio of the border width at that point. Is 'radio' a typo?
Attachment #8662303 - Flags: review?(jmuizelaar) → review-
Thank you for reviewing! :D (In reply to Jeff Muizelaar [:jrmuizel] from comment #150) > Comment on attachment 8662303 [details] [diff] [review] > Part 4: Support dotted/dashed border-radiused corners. > > Review of attachment 8662303 [details] [diff] [review]: > ----------------------------------------------------------------- > > I haven't looked at everything yet, but here are some preliminary comments. > > It might be worth packaging up Point[4] into a Curve or Bezier type. This > will let you assign them instead of having to memcpy. Okay, added Bezier type, that has 'Point mPoint[4]' member. > @@ +262,5 @@ > > +{ > > + Float A = a + b, S = a - b; > > + Float A2 = pow(A, 2.0f), S2 = pow(S, 2.0f); > > + > > + return M_PI * A * (3.00f * S2 / (A2 * (sqrt(-3.0f * S2 / A2 + 4.0f) + 10.0f)) + 1.0f) / 4.0f; > > Where does this formula come from? I should've added the command and the reference for it. It's Ramanujan's approximation: https://en.wikipedia.org/wiki/Ellipse#Circumference http://books.google.com/books?id=oSioAM4wORMC&pg=PA39 Added comment, and slightly modified the formula to reduce the number of division. > @@ +6,5 @@ > > +#ifndef mozilla_BezierUtils_h_ > > +#define mozilla_BezierUtils_h_ > > + > > +#include "mozilla/gfx/2D.h" > > +#include "gfxRect.h" > > Is this include needed? mozilla::css::Corner is defined there, and GetBezierPointsForCorner uses it. > @@ +197,5 @@ > > + Point innerSectionPoints[4]; > > + SplitBezierB(tmp, mOuterPoints, lastTo); > > + SplitBezierA(outerSectionPoints, tmp, ConvertRange(to, lastTo)); > > + SplitBezierB(tmp, mInnerPoints, lastTi); > > + SplitBezierA(innerSectionPoints, tmp, ConvertRange(to, lastTo)); > > Should this be? > SplitBezierA(innerSectionPoints, tmp, ConvertRange(ti, lastTi)); instead of > To? > > Would it be better to have a SplitBezier function that takes a t1 and t2? wrapped those 2 functions with GetSubBezier function. SplitBezierA and SplitBezierB is defined in BezierUtils.cpp with 'static'. > ::: layout/base/DashedCornerFinder.h > @@ +175,5 @@ > > + Point mLastPi; > > + Float mLastTo; > > + Float mLastTi; > > + > > + // Length for each segment, radio of the border width at that point. > > Is 'radio' a typo? Oops, it should be 'ratio'.
Attachment #8662303 - Attachment is obsolete: true
Attachment #8702087 - Flags: review?(jmuizelaar)
Some files are changed from last time, and fixed gfx/2d directory too, as BezierUtils.cpp is moved there and I hit error there. [gfx/2d/DataSurfaceHelpers.h] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/gfx/2d/DataSurfaceHelpers.h#93 > DataAtOffset(DataSourceSurface* aSurface, > - DataSourceSurface::MappedSurface* aMap, > + const DataSourceSurface::MappedSurface* aMap, > IntPoint aPoint); This is not include or namespace tho, this declaration does not match to definition in DataSurfaceHelpers.cpp. [gfx/2d/Quaternion.h] > +#include <ostream> https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/gfx/2d/Quaternion.h#37 > friend std::ostream& operator<<(std::ostream& aStream, const Quaternion& aQuat); ostream is used here > +#include "mozilla/gfx/Point.h" https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/gfx/2d/Quaternion.h#95 > Point3D RotatePoint(const Point3D& aPoint) { Point3D is used here. [layout/base/DisplayItemScrollClip.cpp] > +#include "DisplayItemClip.h" http://hg.mozilla.org/mozilla-central/file/68b33692bed3/layout/base/DisplayItemScrollClip.cpp#l34 > str.AppendPrintf("<%s>", scrollClip->mClip ? scrollClip->mClip->ToString().get() : "null"); mClip is DisplayItemClip. [layout/base/GeometryUtils.cpp] > +#include "nsContentUtils.h" https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/GeometryUtils.cpp#233 > if (nsContentUtils::IsCallerChrome()) { nsContentUtils is used here. [layout/base/nsPresShell.cpp] > +#include "gfxUserFontSet.h" https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/base/nsPresShell.cpp#1088 > gfxUserFontSet* fs = mPresContext->GetUserFontSet(); gfxUserFontSet is used here. [layout/generic/nsGridContainerFrame.h] > + typedef mozilla::LogicalSize LogicalSize; http://hg.mozilla.org/mozilla-central/file/a296d149c69e/layout/generic/nsGridContainerFrame.h#l548 > const LogicalSize& aComputedMinSize, > const LogicalSize& aComputedSize, > const LogicalSize& aComputedMaxSize); LogicalSize needs namespace. [layout/style/nsFontFaceLoader.h] https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/layout/style/nsFontFaceLoader.h#59 > - TimeStamp mStartTime; > + mozilla::TimeStamp mStartTime; TimeStamp needs namespace.
Attachment #8659312 - Attachment is obsolete: true
Attachment #8659312 - Flags: review?(roc)
Attachment #8702088 - Flags: review?(jmuizelaar)
Just removed unnecessary changes, such as TouchCaret.h that is already removed.
Attachment #8684600 - Attachment is obsolete: true
Attachment #8702089 - Flags: review+
also, rebase other parts.
Attachment #8659314 - Attachment is obsolete: true
Attachment #8659314 - Flags: review?(roc)
Attachment #8702090 - Flags: review?(jmuizelaar)
Attachment #8659317 - Attachment is obsolete: true
Attachment #8659317 - Flags: review?(roc)
Attachment #8702091 - Flags: review?(jmuizelaar)
Attachment #8702091 - Flags: review?(jmuizelaar) → review+
Attachment #8702090 - Flags: review?(jmuizelaar) → review+
Attachment #8702088 - Flags: review?(jmuizelaar) → review+
Keywords: dev-doc-needed
Thank you for reviewing :) I'm going to land Part 0-2 after try run, as they don't depend on other parts.
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/04539f076e4587a0975527828bf43e129e8ed0ba Bug 382721 - Part 0: Add missing includes and namespaces. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/1990c87edb47f88fd93e4879d65427974c57fd22 Bug 382721 - Part 1: Fix spacing of simple 2px dotted border without radius. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/399dfde76376dd73e3d3f03e21afb80204bf796d Bug 382721 - Part 2: Split constants for border rendering to BorderConsts.h. r=jrmuizel
Blocks: 379303
can you review Parts 4-5?
Flags: needinfo?(jmuizelaar)
I'm going to finish this review today.
Flags: needinfo?(jmuizelaar)
Attachment #8702087 - Flags: review?(jmuizelaar) → review+
Attachment #8662304 - Flags: review?(jmuizelaar) → review+
Thank you for reviewing :) Will land after testing again.
Keywords: leave-open
(In reply to Tooru Fujisawa [:arai] from comment #165) > Thank you for reviewing :) > Will land after testing again. Sorry it took forever and thanks for being patient and all the work you did.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2881c49e123c204f835953d6c37f004f1549545 Bug 382721 - Part 3: Improve spacing and corner interaction of dashed/dotted border. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2840c13fb097e422df8b293cf6f6e590f4a6c6 Bug 382721 - Part 4: Support dotted/dashed border-radiused corners. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d313145a591f5601a7b568194864b7dcddc11f Bug 382721 - Part 5: Add testcases for dashed/dotted borders. r=jrmuizel
It crashes on the following testcase. https://dxr.mozilla.org/mozilla-central/source/layout/generic/crashtests/730559.html > <!DOCTYPE html><html style="height: 6523790304542em; width: 6207636626031em; box-sizing: border-box; border-style: dotted; -moz-column-width: 20px;"></html> it's a huge box with dotted border, but no radius, so apparently from Part 3. It takes so much time because it tries to draw each dot along super long border. not yet figures out the actual reason of the crash tho. should be better testing a huge box with radius also.
It crashes because of OOM while generating large path. we should flush it after some segment.
the 730559.html's case could be fixed by the comment #170 approach. But, for a large box with large radius (like 6523790304542em), it's not possible to calculate the fitting path in reasonable time, with current approach, as we need to search it with loops. We might need to add some fallback for that case, with less complexity, like using single border-width even adjacent borders have different width. Is there any guideline for the case when webpage specifies unreasonable style? looks like, bug 730559 adds a limitation on the column count. is it reasonable to add some limitation here too?
Flags: needinfo?(jmuizelaar)
(In reply to Tooru Fujisawa [:arai] from comment #171) > the 730559.html's case could be fixed by the comment #170 approach. > > But, for a large box with large radius (like 6523790304542em), it's not > possible to calculate the fitting path in reasonable time, with current > approach, as we need to search it with loops. > We might need to add some fallback for that case, with less complexity, like > using single border-width even adjacent borders have different width. > > Is there any guideline for the case when webpage specifies unreasonable > style? > looks like, bug 730559 adds a limitation on the column count. is it > reasonable to add some limitation here too? Is is possible to look at the clip and restrict the problem?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #172) > (In reply to Tooru Fujisawa [:arai] from comment #171) > > the 730559.html's case could be fixed by the comment #170 approach. > > > > But, for a large box with large radius (like 6523790304542em), it's not > > possible to calculate the fitting path in reasonable time, with current > > approach, as we need to search it with loops. > > We might need to add some fallback for that case, with less complexity, like > > using single border-width even adjacent borders have different width. Also, which specific part of the code do we get stuck in for these very large cases?
Flags: needinfo?(jmuizelaar)
Thanks :) There are 3 places that takes time and/or memory. A) building the path with many parts nsCSSBorderRenderer::DrawDottedSideSlow https://hg.mozilla.org/integration/mozilla-inbound/diff/e2881c49e123/layout/base/nsCSSRenderingBorders.cpp#l1.1399 nsCSSBorderRenderer::DrawDottedCornerSlow https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/nsCSSRenderingBorders.cpp#l1.411 nsCSSBorderRenderer::DrawDashedCornerSlow https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/nsCSSRenderingBorders.cpp#l1.446 This exists both in side and corner, and this is mostly memory issue. They could be improved by skipping paths outside clip. (the clip you're saying is the currently visible or updating area on the screen, right?) B) finding the best overlap for dotted, and the best dash length for dashed DottedCornerFinder::FindBestOverlap https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/DottedCornerFinder.cpp#l1.371 DashedCornerFinder::FindBestDashLength https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/DashedCornerFinder.cpp#l1.295 This exists only in corner, and this is time issue. I'm not sure if this can be solved by clip, without loosing accuracy, as we're searching an overlap/length that satisfies dotted/dashed corner connects smoothly with adjacent sides, that's not related to the clipped area. If we don't have to connect to adjacent side smoothly, like, if inconsistent gap between corner and side are allowed for those cases, we could skip this. C) finding next dot or dash DottedCornerFinder::FindNext https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/DottedCornerFinder.cpp#l1.243 DashedCornerFinder::FindNext https://hg.mozilla.org/integration/mozilla-inbound/diff/6f2840c13fb0/layout/base/DashedCornerFinder.cpp#l1.206 This also exists only in corner, and is also time issue. They are called inside B, and while generating paths, both with loop along the corner. If we have to generate a consistent rendering throughout scrolling, we should calculate whole paths along the corner, or at least up to that point, so clip won't improve much (at most it could reduce the time taken by it to half, in average). So, to reduce the time taken by this, we need some different way. Will try finding out better solution.
(In reply to Tooru Fujisawa [:arai] from comment #171) > Is there any guideline for the case when webpage specifies unreasonable > style? > looks like, bug 730559 adds a limitation on the column count. is it > reasonable to add some limitation here too? I think it is completely reasonable to add a limitation here. It's worth looking at what other browsers do in these more extreme cases.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #175) > (In reply to Tooru Fujisawa [:arai] from comment #171) > > Is there any guideline for the case when webpage specifies unreasonable > > style? > > looks like, bug 730559 adds a limitation on the column count. is it > > reasonable to add some limitation here too? > > I think it is completely reasonable to add a limitation here. It's worth > looking at what other browsers do in these more extreme cases. Okay, will add limitation. About other browsers, here's there result with the following HTML: <div style="height: 6523790304542em; width: 6207636626031em; box-sizing: border-box; border-top-right-radius: 6523790304542em; border-style: dotted; border-width: 1em 2em;">hello</div> Safari: content process stops responding, at least 1 minutes Chrome: corner is rendered as solid style, side is kept dotted/dashed (chrome uses rectangular dotted tho...) will check Edge after downloading VM, but I think Chrome's way seems reasonable.
Edge: somehow ignores height and displays thin box with no radius. when I reduce the height and width to 60000em, content process stops responding, at least 1 minutes I'll ignore dotted/dashed for large corner.
When using the dirty rectangle it's best to just discard elements that are completely outside of the rectangle. Trying to manually clip the geometry to the dirty rectangle can cause the antialiasing to be different depending on the clip which we'd like to avoid. i.e. ideally the dirty rect shouldn't change any of the actual points in the path geometry. Hopefully, using the dirty rect and adding some limits is enough to handle the more extreme test cases.
Thanks :) Now testing with 3 more patches applied. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc9ccfc2deec Part 6 Draw and flush combined path after adding 100 segments to avoid generating too large paths and causing OOM Part 7 Generate path only inside dirty rect for dotted side, it calculates only required area for corners, it calculates until path enters and leaves the dirty rect Part 8 Render too large dotted/dashed corner with solid style
> Render too large dotted/dashed corner with solid style Can we have a hint in the console in this case to help webdevs debugging? (see also #946430 for a similar cause.)
Added the limitation on the maximum number of segment (dot or dash) while generating path inside the following method: * nsCSSBorderRenderer::DrawDottedSideSlow * nsCSSBorderRenderer::DrawDottedCornerSlow * nsCSSBorderRenderer::DrawDashedCornerSlow Now the max is 100, and if it exceeds, it draws the path and continues with new path builder. This fixes the OOM while building the path.
Attachment #8761618 - Flags: review?(jmuizelaar)
Added aDirtyRect parameter to nsCSSBorderRenderer constructor, and use it inside the following methods: * nsCSSBorderRenderer::DrawDottedSideSlow * nsCSSBorderRenderer::DrawDottedCornerSlow * nsCSSBorderRenderer::DrawDashedCornerSlow aDirtyRect can be nullptr, as we don't have that information for nsCSSRendering::PaintFocus, and in that case the following checks are just ignored. In nsCSSBorderRenderer::DrawDottedSideSlow, it draws each dot between |from| and |to|, if the point for |from| and/or |to| is outside of the |dirty rect + radius + anti-aliasing margin|, |from| and/or |to| is changed to fit into the rect. In nsCSSBorderRenderer::DrawDottedCornerSlow, it checks if each dot is inside the |dirty rect + maximum radius + anti-aliasing margin|, and renders only if it's inside the rect. Also, it breaks the loop when it enters and leaves the rect, as once it leaves the rect, it won't re-enter. In nsCSSBorderRenderer::DrawDashedCornerSlow, it checks if a rect that encloses all control points of the dash overlaps with the |dirty rect + anti-aliasing margin|, and renders only if they overlap. Like dotted, it breaks the loop when it enters and leaves the rect. This fixes the performance issue when rendering a bit large dotted/dashed box. Also, this fixes the OOM issue addressed in Part 6 in most case, but as aDirtyRect can be nullptr, Part 6 is still needed.
Attachment #8761621 - Flags: review?(jmuizelaar)
Added the limitaion on the maximum border-radius of dashed/dotted, now it's 100000px, and if it exceeds, the corner is rendered as solid. This fixes the performance issue when rendering too large dotted/dashed corner.
Attachment #8761622 - Flags: review?(jmuizelaar)
Added a warning message for Part 8's case. The warning is shown only once per PresContext, to avoid flooding the console.
Attachment #8761624 - Flags: review?(jmuizelaar)
> +TooLargeDashedOrDottedRadius=Border radius for dashed/dotted exceeds the limit (100000px) and rendered as solid style. Better wording: "Border radius is too large for dashes or dots (the limit is 100000px). Rendering as solid." Also, when this happens, I think it would look better if the *entire border* were rendered as solid, not just the corner. That's not very important, though, since this will hardly ever come up in real websites (who even has a monitor that big?)
(In reply to Zack Weinberg (:zwol) from comment #185) > > +TooLargeDashedOrDottedRadius=Border radius for dashed/dotted exceeds the limit (100000px) and rendered as solid style. > > Better wording: "Border radius is too large for dashes or dots (the limit is > 100000px). Rendering as solid." Thanks :) Is "dashes or dots" better than "dashed or dotted" ? I feel it should be better using the actual value (dashed or dotted). > Also, when this happens, I think it would look better if the *entire border* > were rendered as solid, not just the corner. That's not very important, > though, since this will hardly ever come up in real websites (who even has a > monitor that big?) as border style can be different for each side (top, right, bottom, and left), what we can do is to use solid for the side which contains too large radius. Then, actually, that needs more change than current one (as there are interaction between dotted and others at the corner without radius), so I'd like to keep using current patch's way if it's acceptable.
(In reply to Tooru Fujisawa [:arai] from comment #186) > (In reply to Zack Weinberg (:zwol) from comment #185) > > > +TooLargeDashedOrDottedRadius=Border radius for dashed/dotted exceeds the limit (100000px) and rendered as solid style. > > > > Better wording: "Border radius is too large for dashes or dots (the limit is > > 100000px). Rendering as solid." > > Thanks :) > Is "dashes or dots" better than "dashed or dotted" ? > I feel it should be better using the actual value (dashed or dotted). In this context, "dashed" and "dotted" would appear ungrammatical. (The sentence is in the present tense, so -ed verb forms look improperly conjugated.) You could do Border radius is too large for 'dashed' or 'dotted' style without grammar problems, but "Border radius is too large for dashes or dots" communicates the same thing and is shorter, and this message is already rather long. > as border style can be different for each side (top, right, bottom, and > left), what we can do is to use solid for the side > which contains too large radius. > Then, actually, that needs more change than current one (as there are > interaction between dotted and others at the corner without radius), so I'd > like to keep using current patch's way if it's acceptable. Oh, good point. Fine by me to leave as is, then.
(In reply to Zack Weinberg (:zwol) from comment #187) > (In reply to Tooru Fujisawa [:arai] from comment #186) > > (In reply to Zack Weinberg (:zwol) from comment #185) > > > > +TooLargeDashedOrDottedRadius=Border radius for dashed/dotted exceeds the limit (100000px) and rendered as solid style. > > > > > > Better wording: "Border radius is too large for dashes or dots (the limit is > > > 100000px). Rendering as solid." > > > > Thanks :) > > Is "dashes or dots" better than "dashed or dotted" ? > > I feel it should be better using the actual value (dashed or dotted). > > In this context, "dashed" and "dotted" would appear ungrammatical. (The > sentence is in the present tense, so -ed verb forms look improperly > conjugated.) You could do > > Border radius is too large for 'dashed' or 'dotted' style > > without grammar problems, but "Border radius is too large for dashes or > dots" communicates the same thing and is shorter, and this message is > already rather long. Thanks again :D "'dashed' or 'dotted' style" sounds better for me. to be clear, there are 2 reasons why I feel using 'dashed' or 'dotted' is better: 1. the message is the target of localization I'm not sure if localized "dashes or dots" communicates the same thing as "'dashed' or 'dotted'" in each locale so using keywords and avoid translating those words seems better 2. we cannot point any particular line/rule for the warning message (like, the line 10 of foo.css) because it's detected while rendering, not parsing so using keywords seems to be more developer-friendly, as they can search with them
> 1. the message is the target of localization > I'm not sure if localized "dashes or dots" communicates the same thing as "'dashed' > or 'dotted'" in each locale so using keywords and avoid translating those words > seems better > > 2. we cannot point any particular line/rule for the warning message (like, the line 10 > of foo.css) because it's detected while rendering, not parsing, so using keywords > seems to be more developer-friendly, as they can search with them OK, that's fair. In that case, could we have two messages, one for dashed and one for dotted? That would make life even easier for developers.
Okay, changed the wording, and separated the message for dashed and dotted.
Attachment #8761624 - Attachment is obsolete: true
Attachment #8761624 - Flags: review?(jmuizelaar)
Attachment #8761730 - Flags: review?(jmuizelaar)
Comment on attachment 8761621 [details] [diff] [review] Part 7: Render only dirty area for dotted side and dotted/dashed corner. Review of attachment 8761621 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +987,5 @@ > // to a style context and can use the same logic that PaintBorder > // and PaintOutline do.) > nsCSSBorderRenderer br(aPresContext->Type(), > aDrawTarget, > + nullptr, Instead of passing nullptr can we pass focusRect? That should keep the called code simpler. ::: layout/base/nsCSSRenderingBorders.cpp @@ +2225,5 @@ > + } > + } > + } > + } > + Is this hunk required to avoid OOM or does it just allow us to save time? If we don't need it, I'd prefer to keep it out for now.
Comment on attachment 8761730 [details] [diff] [review] Part 9: Warn about too large dotted/dashed corner. Review of attachment 8761730 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.cpp @@ +806,2 @@ > nsCSSBorderRenderer br(aPresContext->Type(), > + aPresContext, If we're going to be passing in the PresContext let's not pass in the type separately.
Attachment #8761730 - Flags: review?(jmuizelaar) → review-
Attachment #8761622 - Flags: review?(jmuizelaar) → review+
Attachment #8761618 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #191) > Comment on attachment 8761621 [details] [diff] [review] > Part 7: Render only dirty area for dotted side and dotted/dashed corner. > > Review of attachment 8761621 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsCSSRendering.cpp > @@ +987,5 @@ > > // to a style context and can use the same logic that PaintBorder > > // and PaintOutline do.) > > nsCSSBorderRenderer br(aPresContext->Type(), > > aDrawTarget, > > + nullptr, > > Instead of passing nullptr can we pass focusRect? That should keep the > called code simpler. Yeah, it works, as we're checking overlap with margin. > ::: layout/base/nsCSSRenderingBorders.cpp > @@ +2225,5 @@ > > + } > > + } > > + } > > + } > > + > > Is this hunk required to avoid OOM or does it just allow us to save time? If > we don't need it, I'd prefer to keep it out for now. It's to save time. Changed DrawDottedSideSlow to use the same way as DrawDottedCornerSlow, and it's still displayed without any notable lag.
Attachment #8761621 - Attachment is obsolete: true
Attachment #8761621 - Flags: review?(jmuizelaar)
Attachment #8761872 - Flags: review?(jmuizelaar)
removed nsPresContextType parameter.
Attachment #8761730 - Attachment is obsolete: true
Attachment #8761875 - Flags: review?(jmuizelaar)
Attachment #8761872 - Flags: review?(jmuizelaar) → review+
Attachment #8761875 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cfef76a52cea7fac8fa2204c492e32f0e5e4a0 Bug 382721 - Part 3: Improve spacing and corner interaction of dashed/dotted border. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/0863ce40d2f743fae75d2951085bff0f546a9b11 Bug 382721 - Part 4: Support dotted/dashed border-radiused corners. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/63f10496a3e4e3deaa70323f7b63d6e8f500a8cc Bug 382721 - Part 5: Add testcases for dashed/dotted borders. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d2cc53a4d653ee631eb77b565da6d2a8e8cdd5 Bug 382721 - Part 6: Flush path while building long dotted/dashed border. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf602c6e885b6a14b4946eeee44a7f77c5db913 Bug 382721 - Part 7: Render only dirty area for dotted side and dotted/dashed corner. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/ba37839e4fdebd7575845f4ed0b06a9c191c30b2 Bug 382721 - Part 8: Render too large dotted/dashed corner with solid style. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f66208fe851ffd0b03dbc1b2cbe6883d2e6d21 Bug 382721 - Part 9: Warn about too large dotted/dashed corner. r=jrmuizel
sorry had to back this out since this caused perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29956176&repo=mozilla-inbound
Flags: needinfo?(arai.unmht)
hmmmm, so, the code block in nsCSSBorderRenderer::nsCSSBorderRenderer for tweaking |from| and |to| was necessary for Android, with dotted rendering after these patch. Even just iterating along the path is too heavy for it :( will post the patch with previous way.
got r+ed over IRC. will land after extra try run on latest m-i.
Flags: needinfo?(arai.unmht)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5306a743dbdfbb4cd223fdc0498bcf91d5942abe Bug 382721 - Part 3: Improve spacing and corner interaction of dashed/dotted border. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/22e1df7564ca1e04a6e1410e4348336a14d3a62f Bug 382721 - Part 4: Support dotted/dashed border-radiused corners. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/f681dfc661e79c5ea57650effda2d6eb217a36d2 Bug 382721 - Part 5: Add testcases for dashed/dotted borders. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/0d02a845d62dcda3e8b214569093ffcb6ff53632 Bug 382721 - Part 6: Flush path while building long dotted/dashed border. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/8d81a2b35bc0f525319b097a79fa97ff89d4f514 Bug 382721 - Part 7: Render only dirty area for dotted side and dotted/dashed corner. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/04c0b0cde81d17048bd42c4d618985b1cd826641 Bug 382721 - Part 8: Render too large dotted/dashed corner with solid style. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac2613adaeefd45f9cde2d4d16746484a1be5e5 Bug 382721 - Part 9: Warn about too large dotted/dashed corner. r=jrmuizel
It appears this also fixes bug 19963, bug 315209 and bug 379303 upon viewing their attached testcases, and also fixes the data: URL tests mentioned in bug 652650 comment 18. Should we resolve them as FIXED now?
(In reply to Mardeg from comment #202) > It appears this also fixes bug 19963, bug 315209 and bug 379303 upon viewing > their attached testcases, and also fixes the data: URL tests mentioned in > bug 652650 comment 18. Should we resolve them as FIXED now? Please make sure the test cases are added to the testsuite, at least.
(In reply to Mardeg from comment #202) > It appears this also fixes bug 19963, bug 315209 and bug 379303 Bug 315209 is not fixed
Tests for Bug 19963 and Bug 652650 are included (slightly different than original one, for simplicity in reftest) I'm not sure what's the expected behavior for Bug 379303 tho (it just says "pretty"...), dashed/dotted borders are now symmetric and test for that is included. Bug 315209 is indeed not-yet-fixed, and totally different thing I think.
Depends on: 1287076
Doc updated: https://developer.mozilla.org/en-US/Firefox/Releases/50#CSS as well as a comment under the browser compatibility tables, in shorthand/longhand properties involved: https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius
Depends on: 1289828
Depends on: 1294015
Release Note Request (optional, but appreciated) [Why is this notable]: Long-standing issue that (while small) always drew a lot of attention [Suggested wording]: Fixed rendering of dashed and dotted borders with rounded corners (border-radius) [Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius Alternative wording adapted from release info linked in comment 206: "Corners with border-radius and dashed or dotted style is now rendered with specified style instead of solid style" (In reply to Jean-Yves Perrier [:teoli] from comment #206) > Doc updated: > https://developer.mozilla.org/en-US/Firefox/Releases/50#CSS > as well as a comment under the browser compatibility tables, in > shorthand/longhand properties involved: > https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius I updated the CSS/border-radius page again, there was another note that was not updated accordingly.
relnote-firefox: --- → ?
Reproducible crash using current Developer Edition: Open https://bugzilla.mozilla.org/attachment.cgi?id=8644118, and set these options: type: dotted corner: A options: unchecked radius-x: 199 radius-y: 286 top-width: 44 left-width: 0 width: 600 height: 507 Then, quickly drag the left-width slider around, back and forth, repeatedly hitting the 0 end. Tab will crash after a few tries. Example crash: https://crash-stats.mozilla.com/report/index/55a48e5d-3d2f-4244-8c7f-628f02160819 Looks like some kind of race condition to me.
Please file a new bug for that crash and mark it blocking this bug rather than adding new comments here that are prone to being lost.
Attached image Rendering problem (deleted) —
Hi, I did a test at https://bugzilla.mozilla.org/attachment.cgi?id=8644118, and set these options: type: dotted corner: A options: unchecked or checked radius-x: 165 radius-y: 301 top-width: 5 left-width: 0 width: 501 height: 600 I can see artifacts on the left and right (FF 50). There are dots that shouldn't be there. Hope this is still a problem of this bug.
reproduced on Fx 51.0a2
Massamino, I think you should file a separate bug with your testcase, with a "blocks" on this bug. It would also be a good thing if you can simplify it to demonstrate the issue right after opening it. Thanks a lot ! (note: I reproduce on my Aurora v51)
Flags: needinfo?(massamino)
Depends on: 1318625
(In reply to Julien Wajsberg [:julienw] from comment #216) > Massamino, I think you should file a separate bug with your testcase, with a > "blocks" on this bug. > > It would also be a good thing if you can simplify it to demonstrate the > issue right after opening it. > > Thanks a lot ! > > (note: I reproduce on my Aurora v51) I filed a new bug and created my own simplified example. https://bugzilla.mozilla.org/show_bug.cgi?id=1318625
Flags: needinfo?(massamino)
Depends on: 1327811
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: