| Summary: | Vertical-aligned of RenderText should not contribute to a layout. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | ChangSeok Oh <changseok> | ||||||||
| Component: | Layout and Rendering | Assignee: | ChangSeok Oh <changseok> | ||||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, commit-queue, darin, esprehn+autocc, glenn, hyatt, kojii, kondapallykalyan, mmaxfield, rniwa, simon.fraser, zalan | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
ChangSeok Oh
2015-06-12 14:00:03 PDT
Created attachment 254829 [details]
Patch
Created attachment 254830 [details]
Patch
Hrm.. Does this also need to be reviewed by Hyatt? I think it does. I understand the problem but I am not sure this is the best solution and would like his view on that. And maybe Simon’s as well. Comment on attachment 254830 [details]
Patch
This does not seem right to me. Vertical alignment was supposed to be computed for line boxes independently, so if there's a mis-positioning occurring, I think the mistake is in the calling code and has nothing to do with the value returned by the RenderText.
(In reply to comment #5) > Comment on attachment 254830 [details] > Patch > > This does not seem right to me. Vertical alignment was supposed to be > computed for line boxes independently, so if there's a mis-positioning > occurring, I think the mistake is in the calling code and has nothing to do > with the value returned by the RenderText. Thanks for your comment, dhyatt! However I can't follow your comment, especially 'computed independently'. As my understanding the spec, verticalAlign is not something computed, it's a just given thing by content since it is not inheritable. In that context, raw text nodes, i.e RenderText, can't have verticalAlign style, but it does in WebKit. Because it reuses the shared container block's style even though it's not inheritable. I think that's the root cause. (Maybe why webkit does like that is for saving memory consumption?). InlineBox returns its renderer's verticalAlign, so it already seems not that 'independent' from renderer. In other words, it already does something with the value of RenderText. we can't allocate own styles for text nodes with any reason and the verticalAlign is a given style, not inherited one, so that we can't solve the issue by fixing its render style. If that's the case, the next possible approach would be fixing it through a commonly used interface. That's the InlineBox::verticalAlign() in my humble opinion. As you said, if we try to fix the issue by adding a workaround to near caller's place, then we might need to add the extra condition, is<RenderText>, everywhere calling verticalAlign() in InlineFlowBox.cpp. I think that's not a neat way since it sounds that potential verticalAlign issues still remain, because which can lead someone to make easily a mistake. It would be appreciated if you can correct me if any. =) Blink took the similar fix, which regressed and fixed differently. Please refer to http://crrev.com/1178473003#msg19 and http://crbug.com/496850#c7. Created attachment 467452 [details]
All browsers match each other
I think Safari Technology Preview 177 matches with other and even Safari 16.6 is same as STP177.
Do we need to do anything else?
|