Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Remove unnecessary CSS styles on <p> elements #32043

Merged
merged 9 commits into from
Mar 28, 2022
Merged

[web] Remove unnecessary CSS styles on <p> elements #32043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1cfe64a
[web] Remove unnecessary CSS styles on <p> elements
mdebbar Mar 15, 2022
ebb228b
Merge branch 'main' into style_cleanup
mdebbar Mar 16, 2022
0027741
Merge branch 'main' into style_cleanup
mdebbar Mar 17, 2022
76fed45
Merge branch 'main' into style_cleanup
mdebbar Mar 21, 2022
52f276d
fix firefox
mdebbar Mar 21, 2022
f890188
Merge branch 'main' into style_cleanup
mdebbar Mar 22, 2022
71ff5a4
Merge branch 'main' into style_cleanup
mdebbar Mar 23, 2022
7931987
fix safari
mdebbar Mar 28, 2022
442aff3
Merge branch 'main' into style_cleanup
mdebbar Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions lib/web_ui/lib/src/engine/embedder.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,20 +525,15 @@ void applyGlobalCssRulesToSheet(
// TODO(web): use more efficient CSS selectors; descendant selectors are slow.
// More info: https://csswizardry.com/2011/09/writing-efficient-css-selectors

// This undoes browser's default layout attributes for paragraphs. We
// compute paragraph layout ourselves.
if (isFirefox) {
// For firefox set line-height, otherwise textx at same font-size will
// measure differently in ruler.
//
// - See: https://github.com/flutter/flutter/issues/44803
sheet.insertRule(
'flt-ruler-host p, flt-scene p '
'{ margin: 0; line-height: 100%;}',
sheet.cssRules.length);
} else {
sheet.insertRule(
'flt-ruler-host p, flt-scene p '
'{ margin: 0; }',
sheet.cssRules.length);
'flt-paragraph, flt-span {line-height: 100%;}',
sheet.cssRules.length,
);
}

// This undoes browser's default painting and layout attributes of range
Expand Down
69 changes: 3 additions & 66 deletions lib/web_ui/lib/src/engine/text/canvas_paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import 'package:ui/ui.dart' as ui;
import '../embedder.dart';
import '../html/bitmap_canvas.dart';
import '../profiler.dart';
import '../util.dart';
import 'layout_service.dart';
import 'paint_service.dart';
import 'paragraph.dart';
Expand Down Expand Up @@ -148,11 +147,10 @@ class CanvasParagraph implements ui.Paragraph {

html.HtmlElement _createDomElement() {
final html.HtmlElement rootElement =
html.document.createElement('p') as html.HtmlElement;
html.document.createElement('flt-paragraph') as html.HtmlElement;

// 1. Set paragraph-level styles.
_applyNecessaryParagraphStyles(element: rootElement, style: paragraphStyle);
_applySpanStylesToParagraph(element: rootElement, spans: spans);

final html.CssStyleDeclaration cssStyle = rootElement.style;
cssStyle
..position = 'absolute'
Expand All @@ -175,7 +173,7 @@ class CanvasParagraph implements ui.Paragraph {
final RangeBox box = boxes[j++];

if (box is SpanBox) {
lastSpanElement = html.document.createElement('span') as html.HtmlElement;
lastSpanElement = html.document.createElement('flt-span') as html.HtmlElement;
Comment on lines -178 to +176
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom elements behave like divs right? Do you need display: inline (or inline-block) to retain what was special of using a span tag earlier? Or are we just doing display:block on everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point.

The display property doesn't matter anymore. Each <flt-span> contains a single fragment of text that can't be broken into multiple lines.

A single line of text that is positioned absolutely shouldn't be affected by display:block or display:inline.

applyTextStyleToElement(
element: lastSpanElement,
style: box.span.style,
Expand Down Expand Up @@ -253,67 +251,6 @@ class CanvasParagraph implements ui.Paragraph {
}
}

/// Applies a paragraph [style] to an [element], translating the properties to
/// their corresponding CSS equivalents.
///
/// As opposed to [_applyParagraphStyleToElement], this method only applies
/// styles that are necessary at the paragraph level. Other styles (e.g. font
/// size) are always applied at the span level so they aren't needed at the
/// paragraph level.
void _applyNecessaryParagraphStyles({
required html.HtmlElement element,
required EngineParagraphStyle style,
}) {
final html.CssStyleDeclaration cssStyle = element.style;

// TODO(mdebbar): Now that we absolutely position each span inside the
// paragraph, do we still need these style on <p>?

if (style.textAlign != null) {
cssStyle.textAlign = textAlignToCssValue(
style.textAlign, style.textDirection ?? ui.TextDirection.ltr);
}
if (style.lineHeight != null) {
cssStyle.lineHeight = '${style.lineHeight}';
}
if (style.textDirection != null) {
cssStyle.direction = textDirectionToCss(style.textDirection);
}
}

/// Applies some span-level style to a paragraph [element].
///
/// For example, it looks for the greatest font size among spans, and applies it
/// to the paragraph. While this seems to have no effect, it prevents the
/// paragraph from inheriting its font size from the body tag, which leads to
/// incorrect vertical alignment of spans.
void _applySpanStylesToParagraph({
required html.HtmlElement element,
required List<ParagraphSpan> spans,
}) {
double fontSize = 0.0;
String? fontFamily;
for (final ParagraphSpan span in spans) {
if (span is FlatTextSpan) {
final double? spanFontSize = span.style.fontSize;
if (spanFontSize != null && spanFontSize > fontSize) {
fontSize = spanFontSize;
if (span.style.isFontFamilyProvided) {
fontFamily = span.style.effectiveFontFamily;
}
}
}
}

final html.CssStyleDeclaration cssStyle = element.style;
if (fontSize != 0.0) {
cssStyle.fontSize = '${fontSize}px';
}
if (fontFamily != null) {
cssStyle.fontFamily = canonicalizeFontFamily(fontFamily);
}
}

void _positionSpanElement(html.Element element, EngineLineMetrics line, RangeBox box) {
final ui.Rect boxRect = box.toTextBox(line, forPainting: true).toRect();
element.style
Expand Down
76 changes: 0 additions & 76 deletions lib/web_ui/lib/src/engine/text/paragraph.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,51 +759,6 @@ void applyTextStyleToElement({
}
}

html.Element createPlaceholderElement({
required ParagraphPlaceholder placeholder,
}) {
final html.Element element = html.document.createElement('span');
final html.CssStyleDeclaration style = element.style;
style
..display = 'inline-block'
..width = '${placeholder.width}px'
..height = '${placeholder.height}px'
..verticalAlign = _placeholderAlignmentToCssVerticalAlign(placeholder);

return element;
}

String _placeholderAlignmentToCssVerticalAlign(
ParagraphPlaceholder placeholder,
) {
// For more details about the vertical-align CSS property, see:
// - https://developer.mozilla.org/en-US/docs/Web/CSS/vertical-align
switch (placeholder.alignment) {
case ui.PlaceholderAlignment.top:
return 'top';

case ui.PlaceholderAlignment.middle:
return 'middle';

case ui.PlaceholderAlignment.bottom:
return 'bottom';

case ui.PlaceholderAlignment.aboveBaseline:
return 'baseline';

case ui.PlaceholderAlignment.belowBaseline:
return '-${placeholder.height}px';

case ui.PlaceholderAlignment.baseline:
// In CSS, the placeholder is already placed above the baseline. But
// Flutter's `baselineOffset` assumes the placeholder is placed below the
// baseline. That's why we need to subtract the placeholder's height from
// `baselineOffset`.
final double offset = placeholder.baselineOffset - placeholder.height;
return '${offset}px';
}
}

String _shadowListToCss(List<ui.Shadow> shadows) {
if (shadows.isEmpty) {
return '';
Expand Down Expand Up @@ -881,37 +836,6 @@ String? _decorationStyleToCssString(ui.TextDecorationStyle decorationStyle) {
}
}

/// Converts [textDirection] to its corresponding CSS value.
///
/// This value is used for the "direction" CSS property, e.g.:
///
/// ```css
/// direction: rtl;
/// ```
String? textDirectionToCss(ui.TextDirection? textDirection) {
if (textDirection == null) {
return null;
}
return textDirectionIndexToCss(textDirection.index);
}

String? textDirectionIndexToCss(int textDirectionIndex) {
switch (textDirectionIndex) {
case 0:
return 'rtl';
case 1:
return null; // ltr is the default
}

assert(() {
throw AssertionError(
'Failed to convert text direction $textDirectionIndex to CSS',
);
}());

return null;
}

/// Converts [align] to its corresponding CSS value.
///
/// This value is used as the "text-align" CSS property, e.g.:
Expand Down
4 changes: 2 additions & 2 deletions lib/web_ui/lib/src/engine/text/ruler.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class TextHeightStyle {
class TextDimensions {
TextDimensions(this._element);

final html.HtmlElement _element;
final html.Element _element;
html.Rectangle<num>? _cachedBoundingClientRect;

void _invalidateBoundsCache() {
Expand Down Expand Up @@ -165,7 +165,7 @@ class TextHeightRuler {
// Elements used to measure the line-height metric.
late final html.HtmlElement _probe = _createProbe();
late final html.HtmlElement _host = _createHost();
final TextDimensions _dimensions = TextDimensions(html.ParagraphElement());
final TextDimensions _dimensions = TextDimensions(html.document.createElement('flt-paragraph'));

/// The alphabetic baseline for this ruler's [textHeightStyle].
late final double alphabeticBaseline = _probe.getBoundingClientRect().bottom.toDouble();
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/test/engine/host_node_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ void _runDomTests(HostNode hostNode) {
hostNode.nodes.addAll(<html.Node>[
html.document.createElement('div'),
target,
html.document.createElement('span'),
html.document.createElement('flt-span'),
html.document.createElement('div'),
]);
});
Expand Down
4 changes: 2 additions & 2 deletions lib/web_ui/test/engine/surface/scene_builder_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,10 +513,10 @@ void testMain() {
renderedLayers[char] = pushChild(builder, char, oldLayer: renderedLayers[char]);
}
final SurfaceScene scene = builder.build();
final List<html.Element> pTags = scene.webOnlyRootElement!.querySelectorAll('p');
final List<html.Element> pTags = scene.webOnlyRootElement!.querySelectorAll('flt-paragraph');
expect(pTags, hasLength(string.length));
expect(
scene.webOnlyRootElement!.querySelectorAll('p').map((html.Element p) => p.innerText).join(''),
scene.webOnlyRootElement!.querySelectorAll('flt-paragraph').map((html.Element p) => p.innerText).join(''),
string,
);
renderedLayers.removeWhere((String key, ui.EngineLayer value) => !string.contains(key));
Expand Down
4 changes: 2 additions & 2 deletions lib/web_ui/test/html/bitmap_canvas_golden_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ Future<void> testMain() async {
canvas.drawParagraph(paragraph, Offset(8.5, 8.5 + innerClip.top));

expect(
canvas.rootElement.querySelectorAll('p').map<String>((html.Element e) => e.innerText).toList(),
canvas.rootElement.querySelectorAll('flt-paragraph').map<String>((html.Element e) => e.innerText).toList(),
<String>['Am I blurry?', 'Am I blurry?'],
reason: 'Expected to render text using HTML',
);
Expand Down Expand Up @@ -229,7 +229,7 @@ Future<void> testMain() async {
canvas.drawParagraph(paragraph, const Offset(180, 50));

expect(
canvas.rootElement.querySelectorAll('p').map<String?>((html.Element e) => e.text).toList(),
canvas.rootElement.querySelectorAll('flt-paragraph').map<String?>((html.Element e) => e.text).toList(),
<String>[text],
reason: 'Expected to render text using HTML',
);
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/test/html/compositing/compositing_golden_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,7 @@ void _testCullRectComputation() {
final html.Element sceneElement = builder.build().webOnlyRootElement!;
expect(
sceneElement
.querySelectorAll('p')
.querySelectorAll('flt-paragraph')
.map<String>((html.Element e) => e.innerText)
.toList(),
<String>['Am I blurry?', 'Am I blurry?'],
Expand Down
Loading