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

[Web][HTML] Add mirrored characters support for RTL languages #39162

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Jan 26, 2023

Description

Handling RTL text in the HTML renderer was introduced by #26811.

It seems that there is a missing piece related to mirrored characters (see https://www.w3.org/International/articles/inline-bidi-markup/uba-basics#mirroring).

This PR adds this feature by relying on the HTML “dir” attribute.

Before/after

Here is a simplified sample (compared to the one in flutter/flutter#108431) to illustrate the issue:

Code sample
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(),
      debugShowCheckedModeBanner: false,
    );
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const Scaffold(
      backgroundColor: Colors.deepPurple,
      body: Center(
        child: Text(
          "(1)",
          textDirection: TextDirection.rtl,
          style: TextStyle(fontSize: 40, fontWeight: FontWeight.bold, color: Colors.white),
        ),
      ),
    );
  }
}
Before (using HTML Renderer):

Capture d’écran du 2023-01-26 16-44-58

After (using HTML Renderer):
Capture d’écran du 2023-01-26 16-45-48

Implementation choice

@mdebbar
I was not able to figure out if there is already some web_ui code to handle mirrored characters.
So the solution I choosed is to rely on the HTML dir attribute and apply it to “flt-span” which is created for each text fragment (adding it only when a text fragment direction is RTL).

Related Issue

Fixes: flutter/flutter#108431

Tests

Adds 1 test.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 26, 2023
@bleroux bleroux force-pushed the fix_html_renderer_does_not_handle_bidi_mirrored_characters branch from 692b82f to 4dd299d Compare January 27, 2023 05:56
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #39162 at sha 996c681

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. Thanks for contributing this fix!

There are some golden triage issues (bug: https://bugs.chromium.org/p/skia/issues/detail?id=13907). Let's wait a bit to see what the Skia team says. I'll triage them later.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 27, 2023
@bleroux
Copy link
Contributor Author

bleroux commented Jan 27, 2023

The PR looks good to me. Thanks for contributing this fix!

There are some golden triage issues (bug: https://bugs.chromium.org/p/skia/issues/detail?id=13907). Let's wait a bit to see what the Skia team says. I'll triage them later.

Great! I was trying to figure out if my PR broke those golden and noticed all golden add this message 'Computing closest positive and negative image. Check back later.'. It might be related to the Tree being currently broken and some golden are stuck?

@mdebbar
Copy link
Contributor

mdebbar commented Jan 27, 2023

It might be related to the Tree being currently broken and some golden are stuck?

It's not related to tree status. This is a bug in the Gold system where some images lose their triage status after a while.

@mdebbar mdebbar added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Feb 2, 2023
@auto-submit auto-submit bot merged commit 616ecd8 into flutter:main Feb 2, 2023
@bleroux bleroux deleted the fix_html_renderer_does_not_handle_bidi_mirrored_characters branch February 2, 2023 21:15
@harryterkelsen
Copy link
Contributor

LGTM

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 2, 2023
…119859)

* 510ecfa93 Don't rely on timings for dimension_provider unit test. (flutter/engine#39343)

* b1ccae745 Roll Skia from 60242c4ea6a7 to c2d81db3ef41 (5 revisions) (flutter/engine#39344)

* 616ecd8be [Web][HTML] Add mirrored characters support for RTL languages (flutter/engine#39162)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Web][HTML] Parentheses aren't shown in the right place in rtl languages
3 participants