Closed Bug 1274158 Opened 8 years ago Closed 8 years ago

perspective(0) should not be treated invalid

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

Bug 1274158 part 1 - Handle zero perspective gracefully.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1274158 part 2 - Accept zero for perspective.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
Per disscussion in in www-style [1][2][3], treating zero invalid here isn't great. It would introduce incompatibility due to rounding error even if all implementations conforms the spec. The spec isn't sensible on this.

My suggestion is treating perspective(0) as infinitesimal, so that the corresponding component in the matrix becomes infinity. Not sure whether there would be other issues, though.


[1] https://lists.w3.org/Archives/Public/www-style/2016Apr/0028.html
[2] https://lists.w3.org/Archives/Public/www-style/2016Apr/0059.html
[3] https://lists.w3.org/Archives/Public/www-style/2016Apr/0060.html
The one known site bug is Bug 1009150, and fixing this would align us with Edge, Safari and Chrome (at least for this sites behavior).

We have a number of dupes for this site bug (I suspect they correlate to Mozillians flying to SFO :p).
I believe other browsers treat perspective(0) as perspective(infinite) which doesn't make much sense... We need the spec to fix and push other browsers to fix their impls as well.
(sorry, I wasn't clear. I just meant the end goal of getting the site to work, like it does in Edge and Chrome, not the actual mechanics)
This patch makes the website in bug 1009150 work as expected.
Comment on attachment 8780378 [details]
Bug 1274158 part 2 - Accept zero for perspective.

https://reviewboard.mozilla.org/r/71106/#review68604

::: layout/style/nsCSSParser.cpp:7820
(Diff revision 1)
> -    if (((aVariantMask & VARIANT_POSITIVE_DIMENSION) != 0 &&
> -         tk->mNumber <= 0.0) ||
> +    if ((aVariantMask & VARIANT_NONNEGATIVE_DIMENSION) != 0 &&
> +        tk->mNumber < 0.0) {

Are we removing this because we don't expect to re-add any non-negative lengths etc., as it would violate the rules that Tab mentioned in the email thread about open ranges of valid values?
Attachment #8780378 - Flags: review?(cam) → review+
Comment on attachment 8780378 [details]
Bug 1274158 part 2 - Accept zero for perspective.

https://reviewboard.mozilla.org/r/71106/#review68604

> Are we removing this because we don't expect to re-add any non-negative lengths etc., as it would violate the rules that Tab mentioned in the email thread about open ranges of valid values?

Yes, I don't think we want to add positive dimension back again. Specs should be fixed if they requires this.
Comment on attachment 8780377 [details]
Bug 1274158 part 1 - Handle zero perspective gracefully.

https://reviewboard.mozilla.org/r/71104/#review68986
Attachment #8780377 - Flags: review?(matt.woodrow) → review+
Assignee: nobody → xidorn+moz
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f2bc29797f09
part 1 - Handle zero perspective gracefully. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/adb44576fbe4
part 2 - Accept zero for perspective. r=heycam
https://hg.mozilla.org/mozilla-central/rev/f2bc29797f09
https://hg.mozilla.org/mozilla-central/rev/adb44576fbe4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1009150
Depends on: 1316236
Interesting scenario in bug 1316236 where treating zero perspective as near-but-not-quite-zero value led to "bad" behaviour.  I've closed that bug as WONTFIX, but given that this is a simple imgur workflow, we may want to reconsider behaving correctly, and going to behaviour that matches Chrome et al. (0 treated as inf, rather than 0).
See bug 1316236 comment 0
Flags: needinfo?(xidorn+moz)
See Also: → 1316236
Naive question - should unit-less zero even be allowed for perspective?
On a side note, it feels like if we're going to clamp to some epsilon, we should do it for all values smaller than that epsilon, not just for zero.  In other words, if I specify epsilon/2 as the perspective value, it will get through as such, but 0 will be pushed up to epsilon.
(In reply to Milan Sreckovic [:milan] from comment #13)
> Interesting scenario in bug 1316236 where treating zero perspective as
> near-but-not-quite-zero value led to "bad" behaviour.  I've closed that bug
> as WONTFIX, but given that this is a simple imgur workflow, we may want to
> reconsider behaving correctly, and going to behaviour that matches Chrome et
> al. (0 treated as inf, rather than 0).

But I don't think Chrome's behavior is correct here, and the CSSWG doesn't either including spec writter who works for Google. See the links in comment 0.

(In reply to Milan Sreckovic [:milan] from comment #14)
> Naive question - should unit-less zero even be allowed for perspective?

We didn't allow, but in this bug we starts to allow. Per Tab's email, defining value only on an open interval violates CSS's general rules.

(In reply to Milan Sreckovic [:milan] from comment #15)
> On a side note, it feels like if we're going to clamp to some epsilon, we
> should do it for all values smaller than that epsilon, not just for zero. 
> In other words, if I specify epsilon/2 as the perspective value, it will get
> through as such, but 0 will be pushed up to epsilon.

We do that. As can be seen from the first patch, we use std::max to choose the bigger value between the specified perspective value and epsilon.
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: